The node:body and node:summary tokens. What on earth are we going to do with those? Deprecate them?
I think that we can and should deprecate these. Not sure exactly how though. We might need to leave the tokens in and trigger a deprecation message for a long time? We already render an empty string if the field isn't on the node type per #1671420: If no body field, [node:summary] [node:body] etc tokens are not replaced → and metatag relying on this seems like a bug, it could accept a special view mode (like search indexing does) or similar maybe?
We keep storage.body in the node module and change the type to not use summary.
I personally think the end point should be that node module doesn't ship the storage field any more.
In this issue, we are not implementing a queue. That is probably the desired end behavior, but would have been too much here and is not really in scope.
That probably makes this comment out of scope but also reading it made me realise something.
If/when this moves to a 'real' queue, it might need to be a single queue item that holds the various operations to do, then the queue runner starts at the top of the operations, and updates (or replaces) the queue item, and if it can't finish, then what's left is in the queue item for another runner to pick up.
This is because if the operations were split into different queue items, they could end up getting processed out of order.
Committed/pushed to 11.x and cherry-picked to 11.2.x, 10.6.x and 10.5.x, thanks!
Don't usually like multiple commits from a single issue but given skipping the test is an emergency stop-gap let's leave this open and continue here maybe?
Put an MR up.
Tagging for follow-up, if there's an existing issue, would be good to add it to the related issues.
This looks fine to me. Do we need a block-specific follow-up to also look at removing the default field config (that still uses text_with_summary) or is that covered somewhere in the general 'deprecate text_with_summary' metas somewhere?
Agreed that the deprecation should be fine for 12.x, this is only used in one place in core outside tests and the usage inside tests is pretty minimal.
Committed/pushed to 11.x, thanks!
Crediting @graber who actually found the bug and bisected it - we were discussing in slack.
Updated the issue summary with a bit more info.
Maybe this would be OK inside attributes, but we had serious problems with event constants in the past with modules being installed/uninstalled and should not repeat that 🐛 Event class constants for event names can cause fatal errors when a module is installed Active .
Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!
Opened 📌 Consider raising sebastian/diff constraint Active for the follow-up.
Updated the issue summary. Removing the FM tag.
The bc policy isn't very clear here, going to try to update it (relevant bit is here: https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... → )
IMO when we're completely removing an event subscriber, we should just remove the whole thing without deprecation (as we would with a hook implementation).
Latest MR looks good. I feel like we should probably just go ahead here as a quick fix, and raise the constraint in a follow-up, given it would be nice to get this in for the patch release next week.
Given @larowlan's:
I did a search of contrib for classes that extend this where these new return types might be a BC break and could only find on in display suite, that was only adding a new method - and one in htmx which also didn't override any of the methods we're adding too here So I think this change is fine to do in a minor release, with a change record
and
https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... →
I think we're OK to add the return type hints to Attribute here with a change record. It's allowed under the bc policy, and we can be fairly confident it won't break any existing code.
Let's go for 'administer node published status' given #86.
Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!
📌 Views entity rendering should use a lazy builder/placeholder Active is one such case where we can assume it's likely that multiple links/urls are involved.
Another one which doesn't have an issue yet is local tasks/actions.
Those would definitely pick a lot more paths up and be less dependent on what's going on in the rest of the page.
This has been rebased since the bot marked it needs work.
📌 Simplify private file routing Needs work would solve this and bring everything more into line with normal routing, so let's try to get that done first. If for some reason that approach isn't possible, can take another look here.
The discussion in #31-34 still stands, the entity handlers can't use dependency injection still.
However, there is no real reason for Cache::invalidateTags() to exist as a wrapper - the static method predates the \Drupal class iirc. We could deprecate that and use \Drupal::service('cache_tags.invalidator')->invalidateTags($tags); in those places, and use injection everywhere else.
I think the dates in #12 should be 2015 rather than 2026?
It's a shame we don't currently have a way to cover this with performance tests, but it would end up being almost as specific as the new test coverage, and the explicit test coverage added here looks like it should stop this regressing again. Maybe one day - it should be enough of an improvement to show on the dashboard though which is nice.
Would a service locator be an option here instead of the lazy services pass? We've been switching to those in 📌 Replace lazy service proxy generation with service closures Active and similar issues.
Could we increase the requirement in core/composer.json on top of what's suggested in #48 to drop support for 4? Bumping a requirement in a patch release seems bad but we'd be loosening it in reality for the vast majority of sites.
or allow 4/5/6/7 in 11.2 with a release note, but open a new issue to bump to 5 in 11.3.
The only sites that will be affected by allowing or disallowing 4 will be sites that already get 4 installed or try to install some new dependency that only allows 4, so feels like whichever of the above we do, it's going to have minimal impact in practice.
Oh thanks for some reason I mis-read this as against content_moderation in core.
MariaDB 10.6 will be out of community support in mid-2026, e.g. roughly when we're expecting to release Drupal 11.
Since 10.6 there are three LTS version:
10.11 - community support until Feb 2028
11.4 - community support until May 2029
11.8 - community support until June 2028 (yes this is sooner than 11.4 is out of support). 11.8 was released 4th June 2025 so it's quite new.
(From https://endoflife.date/mariadb)
If we follow #8, then that points to requiring MariaDb 10.11, since that's what's included in Ubuntu 24.04
For MySQL itself, it's a bit more vague: https://launchpad.net/ubuntu/noble/+package/mysql-server but de-facto you still get 8.0 it looks like. However I wonder if they're going to switch that mid-release to 8.4 (supported until 2029 according to https://endoflife.date/mysql) as 8.0 goes EOL). So maybe it would be fine to require 8.4 according to #8 if ubuntu installs get updated anyway.
On the specific site I'm testing with, this works if I change 'full' to 'default' when attempting to load the display, but otherwise it still falls back to ::getDisplay(). This probably isn't viable because it's specifically looking for 'full' in ::getDisplay(). Including an xhprof screenshot showing a definite improvement when it 'works' though.
A trait for shared logic in the enums was mentioned in the meta issue. A bit of a cop out - but could we just add that to Drupal\Core\Utility.
But also if 📌 Remove comment and node preview Active ever happens we wouldn't have enough uses to justify a trait.
This was added 16 years ago when we didn't have unit tests or kernel tests, and test runs were fast because there were hardly any tests to run. I wonder if we can keep one functional test for the checkbox being disabled, by adding an incompatible module to one of the existing module install form tests. Then the actual dependency logic we expanding ConstraintTest sounds good. Then we could drop this actual test entirely?
@mstrelan thanks for doing this.
I thought we had the steps for moving a module to contrib in a docs page somewhere, but can't find it. The problem is it happens on average about once or twice per year, but it might be worth trying to put somewhere, maybe next to the deprecation guide or similar?
It's not done yet, but there's a plan to refactor content_moderation on top of workspaces (which should work better with trash module) here 📌 Allow for / implement simplified content workflow with workspaces Active .
I like this idea to block state changes while an entity is considered "Deleted".
Something like this would need an issue against the contrib trash module - could open one and link from here in case a different approach comes up in this issue.
I think this is OK in a minor - if we add a change record and release note for it. The release note can point to the contrib issue.
One comment on the MR.
Added an MR.
This is only used when trying to build/send traces for an open telemetry endpoint so it's not really testable.
Committed/pushed to 11.x, thanks!
Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!
wim leers → credited catch → .
Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!
There's still an open discussion here about moving to a service parameter instead of using $settings.
Committed/pushed to 11.x, thanks!
Couple of comments on the MR.
This was originally added 11 years ago in #211747: Allow specifying version information as part of module dependencies.
Had a quick look at the issue, I think it was actually added 16 years ago? Looks like the cvs commit happened in 2009.
Let's definitely open a follow-up to discuss whether we should remove the test altogether or if it can be simplified further.
Committed/pushed to 11.x, thanks!
ConfigEntityListBuilder and all its subclasses override this and make getDefaultOperations() public. Wild.
Do you know why? Might be worth a follow-up to try to remove the overrides, then we wouldn't need to update them in Drupal 12 except for when we delete them.
All the list builder classes will need to have this parameter added in D12, but not sure if all the list builder classes need to be updated now to indicate the change?
It would probably help anyone subclassing the subclasses if we did that, I don't think we've had a similar situation yet - the commented out added parameters is pretty new in itself.
As for the region names themselves, that's something we can decide and adjust later without rushing.
I think a follow-up to review alternatives for the region names is a good idea, and I'd be happy going ahead with the not-bad-but-possibly-not-the-best-we-can-do names in this issue so that the HTML structure is stable and the noise from too many landmarks is gone.
, it should a progress response (I've done X of Y tasks you've asked me to do)
An issue here is the queue system doesn't have an API for finding out how many items there are in the queue, how many items were added by the current user or anything like this. This means either a generic 'still working...' message (which is probably the most accurate and safest option) or having to implement something between queue and batch API.
Using the queue system would be a big step towards unblocking ✨ Allow package_manager to work with non-web-writable directory permissions Active .
On running down the queue, neither of these actually have code unfortunately but ✨ Add an 'instant' queue runner Needs work and ✨ Add a non blocking batch runner Active may have ideas.
One possibility for project_browser would be when adding items to the queue, add something to the user's session saying they've added something to the queue.
Then when this session key exists, add an AJAX queue runner to the page that processes the queue one by one (using something like the process route that @phenaproxima describes). When it doesn't find anything in the queue, it would unset the session key so that there's no an AJAX request all the time.
Need to allow for the possibility that different browser tabs, or cron, run down the queue as well.
Just to expand a bit:
The current label (title), "Show the same menu markup on every page", does not give enough information, so the user has to read the description (help text) to figure out what it means.
I think that
Add the active-trail class
has exactly the same problem, or is possibly slightly worse - e.g. only a tiny, tiny fraction of experienced Drupal users will know what the 'active trail' class is, so everyone will need to read the description anyway. But by making the active trail concept the main thing, it becomes more important to understand what it is, vs. deciding that 'yes, I do want my main menu to look the same on every page'.
Trying a re-title.
Add the active-trail class
I tried to avoid this when we originally started work on this issue, because the concept of the 'active trail' does not exist in the UI anywhere else in Drupal core and I'm not sure we should introduce it here.
By highlighting it as the checkbox label, it promotes the concept above where I think it should be for users (ideally not at all, but minimally introduced by this MR out of necessity). #33 / #34 show exactly how difficult it is to keep track of the difference between 'active' and 'active trail'.
Yet another option would be to add the configuration option without any UI, the problem with that is discoverability of what can be a very significant performance improvement.
No I think that's fair enough, I think the main question then is how to handle adding and then removing the parameter again vs. making it permanent?
Maybe I'm misunderstanding, but it's throwing \InvalidArgumentException, so there's no deprecation or removal needed for the exception itself.
No I just hadn't checked which exception it was actually throwing properly before typing...
Went ahead and resolved the thread per #9.
#2 looks like it should work.
I took a look at converting EntityFieldManager::getFieldLabels() to use the field map and it doesn't help unfortunately.
The field map doesn't contain the field data itself, only which bundles have a field. Because this includes base fields/properties, every bundle is included in the field map, so you end up calling ::getFieldDefinitions() on all bundles anyway when building views data because it wants the field labels for base fields too.
If we knew which fields had overrides or not and which ones were base vs. configured in advance, we could pre-filter, but that information is from ::getFieldDefinitions(), so short of storing that in key/value too, it's just circular.
Opened an issue anyway since it might help in cases where you really only want the field labels for one field 📌 Use the field map in ::getFieldLabels() Active .
It's complicated by the menu allowing you to show which levels to use. So you can have a four level menu, only show the top level, but the three levels underneath all impact the active trail.
I'm not sure what tests would look like for this (that would actually prevent the regression) except for visual regression tests that we don't have a framework for. If someone does, it would be great to add them here. If there's not a good way to add tests, we should open an explicit follow-up to add them later and add it to the navigation stable meta somewhere.
I'm not too worried about an 11.2 backport, this is a regression for cached pages, but it's a regression from very fast to still quite fast. It'll be barely measurable for requests that don't hit the page cache (partly why we haven't noticed it until now) and it's all CPU rather than i/o. If we end up with something backportable that's great but more interested in the longer term solutions.
Either we revert to container aware or we pull hooks out of event dispatcher (or both).
So I think if we revert to container aware event dispatcher, then we should have almost zero overhead from having hooks in event dispatcher, but we could still move them out for other reasons.
If we move hooks out first, then the regression from not using container aware will be whatever it was in 10.3 when the initial issue to not use it was committed, which seems relatively small but also worth fixing. While I might have predicted the future I also wasn't thinking about cached pages in that comment.
The problem with that issue and the MRs here is they are both just treating the symptom not the cause.
This is true but also I think if we know it doesn't scale to thousands of events and we already have a plan to stop putting thousands of events in it, we should definitely do that.
On the front page of Umami after a cache clear, I still get 15 calls to FieldHooks::entityBundleFieldInfo() - didn't check how many total bundles there are, I think this is from views data as you say.
On Umami this is about 330ms for me, which is not tens of seconds bad, but it's still 330ms that will block rendering of a lot of pages on a site (e.g. anything with a view on it).
For me webform + jsonapi is an extreme case and we should definitely do 📌 Change JSON:API routes from every bundle/field to per entity with arguments Active for that case, but we have a lot of code paths calling EntityFieldInfo::getFieldDefinitions() bundle by bundle and should either try to optimize that or convert cases to using the field map.
Here's a before/after xhprof screenshot with HEAD vs. 🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active .
Note that profiling differs whether you have dev dependencies installed or not, at least partly due to OpenTelemetry SDK, I think this was with dev dependencies installed, but the reduction is about the same even if the totals are different.
So 🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active is removing about 2/5ths of the function calls and about half the wall time.
If 📌 Replace ContainerAwareEventDispatcher with Symfony EventDispatcher Fixed is going to be non-trivial to revert, then we could potentially profile 10.3.x just before/after that commit to see what the impact on cached pages is there. That would give us an idea whether it really is worth reverting or not.
One thing I noticed during debugging that a lot of event listeners we have in core (that aren't hooks) are core event listeners for core events, we could also reduce that list by converting some core events to hooks 😇.
To describe the basic issue:
We add hooks to the event dispatcher via the 'calls' parameter to the event dispatcher, which calls ::addListener(), this happens every time event dispatcher is instantiated, e.g. on every single request.
So even though we don't instantiate the individual hook services, we iterate over every hook implementation in core to create a service closure. This involves many recursive calls to ::resolveServicesAndParameters().
ContainerAwareEventDispatcher on the other hand only creates the service definition lazily - e.g. when a specific event is dispatched.
Even though the original reason we added ContainerAwareEventDispatcher is no longer an issue (e.g. services are not actually constructed for every listener), the sheer number of event listeners in Drupal core now after OOP hooks means that even creating the service closures on every request is causing a measurable performance regression.
Here's a quick go at a revert, it also doesn't work. I get "Unable to find the controller for path "/". The route is wrongly configured." after a drush cr.
It looks from the reddit thread that the resolveServicesAndParameters calls take about 2-3ms - the problem with this on page cache hits is that a page cache hit only takes 2-3ms at most, so it's doubling the request time.
Also 11.1 only has core hooks converted, once you have a site with 2-300 modules, all of which have OOP hooks, that 2ms could turn into 5-10ms or more, which will be noticeable on dynamic page cache hits too (dynamic page cache hits are usually more like 60-70ms, so increasing those by 10-20%).
One issue I'm seeing profiling is this is running for discovered procedural non-hooks still - uploading some dump output from having a look around. And because those go from collection to service closure, it looks like it results in two calls each.
#3225792: Allow deprecation testing to distinguish between major versions when contrib modules run tests → is the issue for ignoring deprecations depending on removed in version. For me the upgrade status support is enough because most contrib pipelines don't enable deprecations anyway, but it would definitely be nice if we could handle it in phpstan too.
With the exception removed, this will be a fatal error.
If we remove the exception itself, then yes, but if we remove the exception throwing and not the exception itself, then it will only be a fatal error if the exception otherwise might have been thrown. And I think that might be fine for a Drupal 12 change because that code can be rewritten to use ::hasField(), then the actual exception can be deprecated for removal in Drupal 13.
We add the 'active' class in JavaScript, which is based on 'is the link pointing to the actual page you're on', but we can't easily do that with the active trail class because it's based on a calculation of where the current page sits in the menu tree - the link can be added to the parent of the page even when the page doesn't actually show in the menu at all, the only way to add it via JavaScript would be to do an AJAX request, or calculate the active trail out of band and add it to Drupal settings or something then have javascript add it from there. It would also be JavaScript that's required to render pages to anonymous users whereas currently zero JavaScript is at all out of the box.
I can see it's coming \Drupal\Core\Entity\EntityFieldManager::getFieldLabels(). But that is only called if there is a field storage for a given entity type. so I assume you actually do have a configurable field there?
That code loops over all bundles just to figure out which bundles have a given field. We already have this information, in the field map. Maybe we can prevent this higher up in the chain?
This isn't how I'm reproducing it but it might be a similar pattern causing the problem.
Uploading an xhprof screenshot that shows the callers (to Drupal\Core\Entity\EntityFieldManager::getFieldDefinitions) , there are 965 bundles on the site (not all webforms, also paragraphs etc. but webforms are about 2/3rds), so each caller with 965 calls to it is looping over every bundle (unless it's an amazing coincidence).
The main culprits are:
Drupal\jsonapi\ResourceType\ResourceTypeRepository::getAllFieldNames() Drupal\jsonapi\ResourceType\ResourceTypeRepository::getFields()
Drupal\jsonapi\ResourceType\ResourceTypeRepository::calculateRelatableResourceTypes()
Drupal\search_api\Plugin\search_api\processor\ReverseEntityReferences::getEntityReferences()
It might be that we need to follow a similar approach to your suggestion above in all four places and add some docs discouraging people from looping over all bundles.
Confirmed that none of the webform bundles have a configurable field fwiw.
It looks like there might be some variation in performance tests after this change, will probably need some debugging, but even with the variation there's an obvious measurable improvement.
There's certainly use cases that would benefit from this, but since XB is primarily for the anonymous users, there's usually less need for highly dynamic blocks.
I'm not sure how this is the case. XB will be used for landing pages as well as entity content, so will include plenty of views listings etc., or 'related articles' / 'read next' content on node pages and similar, and these all benefit very considerably from placeholdering - both via big pipe (potentially for sessionless users in core soon), and even more once we have async rendering in place which is progressing steadily in core. Authenticated users also look at those pages depending on the site, where dynamic page cache will be relied on more, and also benefits considerably from placeholdering.
there isn't a lot of traction there
I think there are very specific structural issues for why Layout Builder issues didn't get attention between 2018-2023, we don't need to go into those here but it was very hard to get movement on any layout builder issues for some time as soon as it was stable. Also layout builder issues haven't been prioritised by many/any people since experience builder began development in 2023/24.
There's also the fact that performance issues aren't experienced by most site owners as a specific identifiable problem they can open or follow an issue for, but as a general 'sluggishness' that they tend to generally blame Drupal for but don't have the capability to diagnose.
Would it make sense to enable memory cache for the field config entity type?
I kind of thought we already do... Would need to look properly at what's going on in config entity loading.
Found another similar issue in EntityFieldManager: 📌 Optimize EntityFieldManager::buildBundleFielDefinitions() Active .
Adding before/after xhprof screenshots from the 10.4 site I found this on, and also the (hacky, don't use it) backport patch I used to get the numbers.
That looks good, agreed the page itself can be moved around later.
@rajab natshah that's what this issue is for deciding. For me personally I don't think there's an urgent need to have it in core because as far as I know it stable and working well in contrib. On the other hand 📌 Allow for / implement simplified content workflow with workspaces Active consolidates/reconciles two conflicting core modules and could potentially simplify aspects of trash module too, so that's the current priority in terms of the general area.
Not sure we'd have to deprecate for Drupal 13 in this case because in general you should not be using exceptions for code control, hasField() already exists and is the actual API to use. The exception is supposed to tell you 'this field doesn't exist, are you sure you used the right field name'. It looks like the goal of this issue is move towards more of a 'garbage in garbage out' where typos fail silently, but fields that may or may not exist are more convenient to deal with. But code that is really trying to make sure it doesn't load a non-existing field because it knows it might not be there should already be using ::hasField() IMO.
A related issue is EntityFieldManager::getFieldDefinitions() - again in the webform case you can have a large number of bundles with no bundle fields at all, this means hundreds of cache sets of empty arrays on cold caches to chained fast.
We could potentially change this to a cache by entity type? Will open a separate issue to look at that.
So this reduces the number of entity queries from 1 per bundle to one per entity type. With webform it's not hard to get to hundreds of bundles (that don't actually have any configurable fields on them), so it can reduce the number of database queries by several hundred on a cold cache. On more regular sites with say 10 entity types and 5 bundles each it would still reduce 50 queries to 10.
Also because the first time it loads the field config entities it's for the entity type, not each bundle, it results in less database queries/cache sets / cache gets from the config entity storage too.
Performance tests are showing the improvement (by failing - haven't updated them yet), however seeing a _lot_ of kernel test failures which is likely to be the memory cache entry not being invalidated properly yet.
Opened 📌 Move MainContentViewSubscriber::WRAPPER_FORMAT somewhere else Active because it felt odd seeing the event subscriber constant referenced.
Committed/pushed to 11.x, thanks!
It's not clear to me what the use case is for overriding the header. What are you trying to change?
Also if ::revisionOverview() is overridden, calling parent::revisionOverview(), couldn't the header be changed there before returning.
One question on the MR about the removed version.
Part of me wondered whether this could have been a generic enum in core somewhere providing the same options, but node preview, comment preview, link title text does not really justify this at all, so agree it's better to just put them on where they're used.
I'm fine with doing the constants here or a follow-up, but if we do them in a follow-up let's open an issue for that prior to commit.
Committed/pushed to 11.x, thanks!
This doesn't feel like the right place to fix the problem, shouldn't this step also reset the cache?
than config is updated, so User entity has now new fields definitions.
The type error is correct here IMO, instead contact module should not be passing an invalid value.
Couple of questions on the MR.
Also this feels like it's going to leave file_usage in strange state - e.g. won't there be usage entries that can now never be removed?