@berdir I'm a bit confused by #95 - there are a lot of performance test changes in the MR (showing dozens of queries reduced), do you mean test changes relative to earlier versions of the MR as opposed to relative against HEAD?
Only the actual phpunit 12 update will throw the exception, so for me I think it's worth preparing core for that update as much as possible, and then we can make a final decision when we get there.
The main question is if we're able to jump to phpunit 13 in Drupal 12 without too much disruption. At the moment this will be the most disruptive change, but it's compatible with phpunit 11.
We could maybe reword the change record to be a bit more open about when an exception will start getting thrown, but I think it's good if people start adding the attributes asap.
I think getting Drupal CMS tests onto the public dashboard is probably the next step here, we did the prep work in linked issues from 📌 [META] Track: Proposal for performance testing Needs review but it was never successfully sent - it's probably gitlab ci variables not making it into env or similar, but that's not particularly easy to debug from an MR.
If Drupal CMS still has some default ddev config, then yes I think this still makes sense to do.
📌 Optimize EntityFieldManager::buildBundleFielDefinitions() Active was found via the same profiling as this issue, and does a similar-ish thing (will conflict because it affects the same performance tests).
Pushed a commit from the attempt to add this in the other issue. iirc this version worked, but we need to decide whether we need bc for the field storage property and if so how to do that.
My only concern is that we essentially kind of optimize for bad patterns. These patterns exist a lot, such as getFieldLabels(), jsonapi and more.
Opened a plan issue to track some of those related issues 🌱 Try to avoid iterating over all bundles and fields Active . Also 📌 Try to add static caching for field storage / config entities Needs work for the field config static caching.
I can't remember what my exact testing steps were for #14 (this is why automated performance tests are good..).
I double checked and got different numbers again.
https://drupal-dev.ddev.site/en/recipes/super-easy-vegetarian-pasta-bake
drush cr and then loading the node page with xhprof. This was logged in as admin with the navigation module enabled instead of toolbar.
That gave me 18 calls to Drupal\field\Hook\FieldHooks::entityBundleFieldInfo and 18 database queries, the queries taking 51ms out of 59ms.
With the MR applied, I get 8 queries that take 24ms - so a 25ms improvement, whole method took 32ms.
All on ddev and profiled with xhprof.
That makes sense - applied the suggestion. MR should be green but ran into a couple of flaky tests.
Oh I think we ran into that already somewhere and talked about making writes into deleted, would need to find again though. Either way it's a good point and we'd need to make sure it's specifically accounted for.
I thought about that, but I was concerned about a recent slack discussion with @mglaman on edge cases such as update cache overrides, that disable write/read and just keep delete
In those cases it should be no-op afaik.
I still have some vague concerns around this and the whole change, not nothing specific to hold this up.
Yeah it's been a bit like this with every issue, had no idea text_with_summary was so deeply embedded everywhere in core until we properly tried to remove it.
MR looks good to me - agree some of those tests will eventually have to move to the text_with_summary module but that can happen later on.
Committed/pushed to 11.x, thanks!
#7 is fair enough, also this isn't a route that anyone is likely to reference so not really worth trying to figure out anything complex.
Committed/pushed to 11.x, thanks!
Years ago when all the hooks vs events discussions were happening I would argue that putting hooks in the container would make it too big, and then I convinced myself that wasn't a problem or at least worth it with OOP hooks. Turns out it was a little bit of a problem but a fixable one!
I left one tiny comment on the MR about whether we can pre-emptively set the cache item when we have the data we know is going to go in there. Everything else looks really good here.
I think we should open an issue to look into partial caching (e.g. similar to how pre-OOP hooks did it), - it would allow skipping lot of admin form alters etc. especially if we were able to do something like key the cache by whether there's a session or not. And/or whether there's a way we can reduce hook invocations on dynamic page cache hits. Tagging needs follow-up for those.
But it seems like there'll be a small saving there anyway because the data structure is simpler.
Also tagging for 11.3.0 release highlights because I think we can say 'Significant memory savings in the OOP hooks implementation' or something among other performance improvements.
Committed/pushed to 11.x, thanks!
Pushed a commit to switch from the phpunit annotation to attribute to unbreak head.
#107 shows we can't use the block titles, but I think we could add back the hidden title per #105 and it's (hopefully?) a one-line change to the template.
I don't have a strong opinion on whether that should be explored here in a follow-up, but would be very happy if we could unblock this again.
This can be reviewed again. If we think the static cache is OK to defer to a follow-up, I'll open that and put the WIP code in an MR there.
Tried adding static_cache: TRUE to FieldConfig entities but there's a problem - shows up in FieldStorageCrudTest.
FieldConfig entities set the equivalent FieldStorage entity as a property. This means if the FieldStorage entity is changed, any FieldConfig entity referencing it will need its static cache invalidated.
I tried changing the property to just the ID and loading the FieldStorage entity each time, but that undid the performance improvement.
This seems worth tidying up, but it doesn't affect the main performance improvement here, so I personally think we might want to open a follow-up for that. I committed my (failed) attempt and reverted it - could split that to a new issue and MR.
Adding 🐛 Config overrides are loaded for English even when translate_english is false Active as a related issue, not because the code is related but because it affects the same performance test scenario and I nearly forgot I'd opened this one already.
The container parameter approach saves an extra cache get, not to be sniffed at.
Ran into a weird nightwatch test, turns out that locale.settings had no translate_english value in that test at all, maybe because it's testing the installer? Either way I added a check that the config actually exists before altering the container.
The cache lookup reductions do look nice, to be fair, that's only going to be true for for one language though.
This is true but I think we could theoretically at least not load overrides for whichever language is the default language (on the basis that the configuration will be in that langauge). But it would be dependent on what happens in 🐛 Configuration language being overwritten during module install Needs work and related issues.
If we add tokens later, would we not still have to support the placeholder logic being added here?
Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!
Yes this is the downside of 🌱 [meta] Just in time updates Active but I think the other reasons to go in that direction are enough to outweigh it.
This is leaving the ::setNewRevision() in ::entityPresave(). I think we either need to remove it, or add a comment to explain why we call it twice in two different hooks now.
I think we've tended to run against the minimum supported version (e.g. 8.3). We can definitely run against 8.5 once Drupal 12 is open (in a couple of months-ish). Running phpstan against multiple versions is probably worth an issue, I guess it will be OK if we don't need to worry about different baselines.
Committed/pushed to 11.x, thanks!
#12 makes sense and explains some of my confusion.
Committed/pushed to 11.x, thanks!
This is a tricky one but it looks good. Committed/pushed to 11.x, thanks!
Given we don't know where this might be used, and those places will need to write an upgrade path, I think the deprecation should be for removal in Drupal 13.
You could try to reproduce the steps in #9. The exact numbers will differ every request, but before/after applying the MR should show a big difference.
#3526080-9: Reduce write contention to the fast backend in ChainedFastBackend →
Committed/pushed to 11.x, thanks! Better to get this in asap.
I think we should probably backport this to 10.6.x too, so moving to 'to be ported'.
Moving this to critical, while we will end up with the same code running or not running, it would be good to resolve this asap especially since this is likely to lead to lots of duplicate reports.
We deprecated annotations already, but for removal in Drupal 13. https://www.drupal.org/node/3522776 →
The only remaining annotations usages in Drupal 11 are in some migrate plugins (which are going to be removed in Drupal 12) and the bc layer for annotations.
For Drupal 11, I think our only option is to fork Doctrine Annotations and un-abandon it to suppress the warning, we've had to fork entire dependencies before (for the Zend/Laminas change iirc).
For Drupal 12, we have 🌱 [policy, no patch] Allow both annotations and attributes in Drupal 11 Active to make annotation discovery opt-out, we could also make it opt-in or bring forward the Drupal 13 removal to Drupal 12, although that would need an announcement and it would need to be announced quickly.
As @berdir pointed out in that issue the security surface is minimal, the main issue is support for newer PHP versions, which could get worse the further into Drupal 12's release cycle we are.
Ahh good point! But this means that given they're now not navs, we could introduce an aria label or hidden title per #98 without them becoming landmarks again.
Added a suggestion on the MR to bring back the hidden titles, not sure the best way to do aria labels there.
Whoops - pushed the enum (still completely untested).
Inverting the logic is fine and sounds good, didn't give it loads of thought, we need to update the path alias and entity loading code to return the suspend to see an effect, but those are the only two at the moment anyway.
It's unfortunate that #63 directly contradicts #42.
#42:
it isn't. But @lauriii has explicitly stated (implicitly on this issue, explicitly in a private Slack referenced by #35) that data integrity MUST be possible to violate for non-default revisions and auto-saves. Which I personally disagree with, but I don't get to decide it.
#63:
Because of this, we should prevent deletion with these uses:
...
Components used in forward/draft revisions (content editors are actively working on)
If the disconnect was in terms of priorities/follow-up issues, this would have been easy to communicate at any time during the issue, but it wasn't done until after I posted #44 and the follow-ups.
🐛 Prevent deleting Code Components if there are usages in forward revisions Needs work exists now, but there was no @todo or any reference to forward revisions at all in the MR that was committed here or any of the discussion beforehand.
Once again, when this kind of information and decision making is buried in private slacks, it makes it impossible to for other people to follow issues or contribute. When people run into issues on their own websites, or try to add features to Canvas later, who don't work for Acquia, this will cause serious problems, as it has done with multiple core initiatives in the past. It should not be something that those of us trying to help have to demand after the fact.
@kentr thanks that's really useful.
So, to me to match the issue summary I think we should probably do #99.2, which would mean changing the nav for the individual menus into div. And the basis for this would be that we consider the navigation sidebar to be 'three (or more or less) menus in a trenchcoat' e.g. just one navigation element that happens to be subdivided due to site configuration and defaults.
I think it then might be worth opening a follow-up to see if there's a better way to represent that subdivision than divs, but that wouldn't be stable blocking then.
@mfb ah OK - but doesn't that mean nothing would replace that placeholder then?
If this works we'd want to implement it in the other fiber loops in core.
We can't mandate a return value for Fiber::suspend(), so it needs to be 100% optional and have a default behaviour when NULL or something other than the enum is returned.
I think the idea of fibers that suspend without kicking off anything async (e.g. the path alias and entity loading issues) is fairly unique to Drupal since it's not actually using anything asynchronous at all, but it's also going to be the most common case by volume even once we've got async http request and database query support in core.
Pushed an untested commit to explore communicating to the Fiber to the caller.
I have no idea how close the event loop issue is and if it will allow to check that.
It probably would, but the existence of this code in ::executeInRenderContext() is currently blocking it because Revolt has no equivalent (according to @kingdutch).
. Could we pass around some flags, like FiberContext::WAIT or FiberContext::RESUME
This is definitely worth a try.
Not sure what you mean by this - the non-javascript placeholdering has always worked fine - the placeholders are replaced on server side.
...
I think it would be best to avoid throwing here - just don't try to process the target if it can't be found.
What I mean is - I would assume that if something has already been replaced server side, then it would not be sent to the browser, so would not be a target at all.
@joelpittet I think the problem is that a nav with an aria label becomes a landmark, which is what we've been specifically trying to avoid since
#3452724-56: Navigation side bar and top bar should have appropriate aria labels →
. I'm basing this on https://www.w3.org/WAI/ARIA/apg/patterns/landmarks/examples/navigation.html
So if we add the aria labels back, we go back to 7 or more landmarks instead of two.
The options then would be:
1. Continue to use nav without a label, because the nav elements are wrapped in another landmark, which is what the MR was previously doing when RTBCed.
2. Stop using the nav element for the individual menu items.
If we can agree that the approach results in the correct number of landmarks, I'd prefer us to move the discussion of the exact markup for individual menus to a follow-up probably.
@quietone is right that we already do this - so update status should warn sites when they're on an unsupported release.
Given that, I think this is a request for the drupalorg project - e.g. when you visit https://www.drupal.org/project/drupal/releases/10.3.14 → you would see an indicator that the release is from an unsupported branch. Nothing specific to do for Drupal core afaict.
I think we discussed doing this once before but I can't find the issue.
Marking the branch unsupported would also mean that update status will flag the releases as unsupported.
When we only supported minor releases for six months this probably would have been too aggressive, but now it's 12 months it seems like something we should consider.
I think this would mean that no other placeholder strategy would find the placeholder either.
Wondering if bigpipe's placeholder creation should try to special case no script placeholders somehow.
If I'm reading correctly, it was removed in https://git.drupalcode.org/project/drupal/-/merge_requests/11122/diffs?c... which removes the aria-label from navigation-menu.html.twig altogether - this is because that template is for the nested individual menus (admin, user etc.) within the navigation toolbar, not for the toolbar itself - and in that case it would be 100% intentional to reduce the labels down to only the main side and top bars and not the nested items.
Code here looks good to me - didn't do a very in-depth review yet.
I think to remove the render() call we'd need to do something like:
1. Use the API everywhere applicable in core.
2. A CR to say that transferring metadata from queries to render arrays will be required (probably in 13.x)
3. Probably remove the actual ::render() call only once we've done some related issues like the ones linked from 📌 Refactor the render context stack to avoid a static property Active .
EntityWorkspaceConflictConstraintValidator restricts editing content to a workspace it's already in, or a descendent workspace. I think that descendent workspaces can have edits to additional entities, but not different edits to the same entity - but that's from memory and haven't used/reviewed it recently.
✨ Ability to edit content in live workspace when it has been edited in other workspaces Active is the core issue to lift that restriction.
https://www.drupal.org/project/workspaces_parallel → supports multiple workspace editing of the same entity in contrib.
I'm not sure if WSE also supports it in a different way or not yet, have a vague memory it lifts the restriction but gives you a big warning when you try to cross-publish but that could be wrong/outdated.
This looks reasonable. A bit surprised that we don't see it more often, but the test coverage shows it can happen.
Committed/pushed to 11.x, thanks!
#8 is a good explanation for this being OK.
I updated an inline comment via gitlab suggestions just above the change here - it was still talking about 'safe string' objects which was an early Drupal 8 concept that eventually became MarkupInterface. We're slowly moving towards Stringable elsewhere and this is consistent with that, the conversion from markup to plain text happens immediately after that line, so any HTML in there will be escaped - we're not actually using MarkupInterface to preserve markup in this specific case, it's just saving people casting them to string (and escaping) themselves.
This looks good and indeed very similar to the node one. text_with_summary makes even less sense for blocks, must have been a copy and paste a long time ago. Much less assumes that block body fields exist than nodes so this is pretty low risk (hopefully that's actually the case).
Committed/pushed to 11.x, thanks!
Two more potential issues, more annoyances than showstoppers:
1. Language module doesn't depend on locale. We can have a soft dependency when the config object doesn't exist but it's a bit smelly.
2. DrupalKernel definitely doesn't care about and shouldn't know about locale, system.site is... questionable but more valid.
Locale module could probably set a container parameter in a service provider, and then language module could look for that container parameter but allow for it not being set. This resolves #2 but not #1. That possibly feels like as far as we should go in this issue, then open follow-ups for the rest.
Putting it in the container is a good idea and would prevent the circular dependency. However if I'm right in #13 we'd miss the opportunity to skip override lookups when the default language isn't English. But that might be a follow-up postponed on 🐛 Configuration language being overwritten during module install Needs work given one of the proposed solutions in that issue is to essential enforce that the default configuration language remains English whatever happens. If we limit the optimisation to English here the container parameter would be a clean way to do it.
This works now.
I am thinking though that instead of locale's translate_english setting, we might want to instead add $settings['translate_default_language_config'] - that would avoid getting config in a config overrider, and it would also mean that if the default language is e.g. French we could avoid looking up config overrides in French then.
Not sure if we can get a way with defaulting that to false though, but it seems pretty odd to use config translation to translate config for the default language, so... maybe?
There are two open issues in the issue summary. Moving to a plan.
@me
I'm a bit hazy about whether $this->language is really guaranteed to be the default language in all cases, because otherwise how does ::loadOverrides() work, so it's checking everything it can.
Read a bit more - it's not guaranteed - it can be set dynamically during language negotiation or similar.
Turns out #7 is a 'we were both right' situation - we have to translate English when it's not the default language, and we also have to translate English when locale's 'translate_english' setting is on, even when it's the default language - at least as far as two tests are concerned. I'm not sure whether there's a real use-case for translating English in config when it's the default language - why not update the config instead? But if we wanted to deprecate that or stop it it feels like something to decide independently of trying to speed things up when it's not enabled unless it somehow becomes blocking.
Getting config values within a config overrider is not fun - easy to end up in an infinite loop. I used the raw storage that's already available. Better ideas welcome. However the performance improvement here is dramatic on cold caches and it's also a small improvement on warm/warm-ish caches.
Tests are green now. Needs some extra comments.
This looks good, agreed we should leave any deprecation of ::merge() for a separate issue, but maybe good to open that?
I guess my only question would be what the chances are of a third party driver that can't handle this, but they could probably fall back to a merge query or something?
Rebased and implemented #8. I'm a bit hazy about whether $this->language is really guaranteed to be the default language in all cases, because otherwise how does ::loadOverrides() work, so it's checking everything it can.
I removed the performance test changes here because they're out of date, seeing what other test failures do or don't happen before worrying about those, but ran one locally and results are similar to #4.