However, there is a tradeoff here. If there are sites out there that frequently update specific block configurations (not content blocks, not menus, just block config entities) then this will now invalidate every single block. I *think* that's not very common, but I have no data on that.
I've come across some horrific situations with block content entities before, like breaking the layout builder optimisation to mark them as not re-usable, so that thousands of block plugins were being loaded on every request. But don't remember ever seeing serious problems with frequent updates of block config entities or thousands of them etc. Doesn't mean it's out there but it would be a new horror, not a familiar one.
* There might also be possible API changes from this. Several tests had to be updated because they were directly updating the underlying block configs instead of going through the config entity, but that was always discourages as it bypasses hooks and other things such as the list cache tag. But maybe people invalidated cache tags directly or something.
This all sounds OK with a change record to me. The worst case is a site needs a manual cache clear.
@fabianx suggested '#placeholder_strategy => ['single_flush'] as a generic way to avoid bigpipe for a placeholder. That would allow other blocks/elements to opt out of bigpipe too which could be a good thing.
I couldn't see a straightforward way to allow a specific placeholder to target a specific placeholder_strategy - because placeholder strategies don't have IDs or anything, they're just tagged services, and they could theoretically be removed by contrib etc.
However, if we add support in big_pipe specifically for 'no_big_pipe', and allow #placeholder_strategy to be used in general, then that gives a way for different placeholder implementations to support different kinds of placeholder_strategy hints. This might be overly flexible but also it was very, very easy to implement and doesn't require any API changes.
We won't be able to require multiple import maps until Firefox ESR supports it, but I think if we're only adding the API for contrib to use initially, we could assume multiple import map support and just not use it in core until it's available in ESR.
StandardPerformanceTest is missing an assertion on ScriptBytes for authenticated users. Added that and ran the tests only job:
https://git.drupalcode.org/project/drupal/-/jobs/4436719
1) Drupal\Tests\standard\FunctionalJavascript\StandardPerformanceTest::testStandardPerformance
Asserting ScriptBytes 122689 is greater or equal to 6000 and is smaller or equal to 7000
Failed asserting that 122689 ( is equal to 6000 or is greater than 6000 ) and ( is equal to 7000 or is less than 7000 ).
115kb of JavaScript reduction for authenticated users out of the box.
I've refactored the core dashboard (https://gander.tag1.io/) to require less frequent test runs. It's currently relying on hourly tests of 11.1.x and 10.4.x instead of every 15 minutes, but theoretically we could run tests every four hours or twelve hours and it should still work now.
Next step for Drupal CMS is 📌 Send opentelemetry traces to the performance dashboard Active . Once that issue lands, we should be able to set up a pipeline schedule to populate the dashboard.
The remaining step after this would be adding some additional test coverage for optional recipes. I'm wondering if we can enable every optional recipe at once. InteractiveInstallTest installs all the recipes shown in the UI installer, but this isn't every optional recipe shipped with Drupal CMS. Not sure if there's a clean way to install 'everything in the recipes folder'
This method doesn't do any http requests, so I think it could move to a kernel test?
Committed/pushed to 11.x, thanks!
This doesn't cherry-pick cleanly to 10.5.x so leaving fixed in 11.x.
Going to postpone on that issue.
I think we need to consolidate the theme hooks first, some like #theme links already have issues to switch to #type item_list with a theme suggestion, which would be cleaner than a new #type links element.
Because performance tests are already split out into their own job, this hardly needs any changes.
The next step would be to set up a pipeline that sends the otel data.
I think it would be good to provide an alternative to MapItem like JsonMapItem. Do we need ✨ Add "json" as core data type Active for that?
Would be good to get this done, so that in turn we can do 📌 Replace links.html.twig with item-list--links.html.twig Needs work .
We can deprecate variables in preprocess now, so should be able to do that, then actually remove those variables from the template in 12.x
OK let's postpone this on 📌 Remove title and wrapper div from item-list.html.twig Needs work
I think this specific issue might be a duplicate of 📌 Replace links.html.twig with item-list--links.html.twig Needs work
+1
Yes it is.
@larowlan how would #37 work with big pipe and AJAX?
@karthik.m the first step here would be to actually review Drupal core for use of the functions.
For example core has two usages of proc_open(), but both of those are in combination with the Symfony Process component, so it's not clear whether those have already been looked at and you're suggesting further changes, or whether checking what core does is still @todo.
I think it's fine to do the two parts either in this issue on in two issues - since the module is hidden there's no user-facing confusion to worry about, we just want to enable it for everyone and clean it up safely for any sites that enabled it already.
I think there's two parts:
1. Ignore the feature module in navigation - should be removing a condition or two.
2. We can mark the feature module itself obsolete https://www.drupal.org/docs/core-modules-and-themes/deprecated-and-obsolete →
For #2, need to add a post update that self-uninstalls the module, and a hook_requirements() that prevents it being installed. Then we can eventually delete the module when all possible sites have had to update to the version that made it obsolete (probably in 12.x). I don't know if there are any currently obsolete modules in core, probably not, but there should be in either 10.x or 9.5.x branches to compare against.
Opened 📌 Refactor the render context stack to avoid a static property Active .
I think #4 is very similar to 📌 Entity lazy multiple front loading Active - in purpose if not implementation, and also 📌 Add PHP Fibers support to BigPipe RTBC .
That leaves #5.
#2 is definitely still relevant, render context is currently tracked in a static property, although we should open a dedicated issue for it.
If the top bar itself is ready to go, should we not remove the feature module and always enable it when navigation is enabled? It's only a feature toggle anyway.
catch → created an issue.
That comment suggests that $submitted_configurable needs to be TRUE in order for that logic to be skipped, that might be the specific problem you're seeing?
I think we need to add a 'computed' (extra) field for 'submitted', once we have that, we can deprecate adding it directly in templates.
Adding another related issue.
Given 📌 [meta] Deprecate Toolabr module Active I think we should probably not do this, and focus effort on 📌 Replace Contextual Links BackboneJS usage with VanillaJS equivalent Needs work instead.
📌 [meta] Deprecate Toolabr module Active will be unblocked soon, which will leave this as the last remaining usage of backbone in Drupal core. It would be great to be able to remove the backbone library from Drupal 12. Bumping this to major.
Explicitly postponing this on navigation being stable, but that is close per 📌 New Toolbar Roadmap: Path to Beta & Stable Active .
We will need to enable navigation in the standard profile and umami in order to actually deprecate toolbar, but there might be some other things that could happen in the meantime like making sure that no tests have explicit dependencies on it.
Switching 📌 [meta] Convert all core plugin types to attribute discovery Postponed to a related issue and marking this explicitly postponed on it because we need to finish all the other issues in that meta before we can start here, and it's not blocking steps like deprecation.
Looks great now, really nice to see this one ready.
Committed/pushed to 11.x, thanks!
If we make the editor config entity type statically cacheable (if it's not already), would we need the dedicated static cache here at all?
I thought we'd already done that for all config entities but #1885830: Enable static caching for config entities → is still open, however individual config entities can opt in.
I think we should move any of the forgotten/missed conversions from this issue to a spin-off.
I think the actual conversion is done now that 📌 Convert MigrateSource plugin discovery to attributes Active is in.
Next steps are 🌱 [policy, no patch] Allow both annotations and attributes in Drupal 11 Active and 📌 Triggering deprecations for plugins using annotations when core plugin type has been converted to attributes Needs work afaik.
This looks good.
Some of the test coverage added depends on migrate_drupal which we're intending to deprecate for removal in 12.x but it won't be the only test coverage depending on migrate_drupal so can be handled as part of that process.
There are a couple of things that are quite complex here but they're covered by existing open issues and/or the eventual deprecation of annotations.
Committed/pushed to 11.x, thanks!
This should be possible to do now.
One comment on the post update, overall this looks good though.
The original discussion about the feed is here: 🌱 Project messaging channel in core initiative Fixed
The vast majority of Drupal users do not have accounts on Drupal.org, and likely do not regularly follow Drupal project news, or engage with communications channels that promote important updates, news and events, or support for the community and association.
Project messaging channel initiative
The goal of this initiative is to provide a channel for messaging about the Drupal project within the administrative interface of core. This channel could provide information about upcoming releases, new feature highlights, events like DrupalCon, and promote support of the project via Association membership or other programs.
For me, Drupal CMS releases fit into that goal pretty well, because it is project news. It's not really supposed to be relevant to the site in most cases, but the site owner, except for maybe something like 7.x and 10.x EOL announcements because those really only affect people running sites on those versions.
I can't think of a use case for the operation other than for user accounts.
MR looks good to me, but unfortunately needs a rebase.
Committed/pushed to 11.x and cherry-picked to 10.5.x, thanks!
I think I agree with @quietone's review - doesn't look like it loses any information from the error message, and moving one line earlier seems fine too.
\Drupal should always be used in functional tests because it's impossible to keep the test container and the site-under-test container perfectly in-sync with a class property.
See 🌱 Use \Drupal consistently in tests Needs work for discussion.
I don't understand why serialize() is being used here and there doesn't appear to be an explanation either in the MR or the issue summary - can't think of any situation where we'd want to do that either.
I think we should be able to add these methods to the existing interface under the 1-1 rule.
See https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... →
Needs a rebase.
It looks like #31 needs to be answered here and the issue summary updated based on the results.
If the file is actually invalid, then I don't think we need to suppress the error in that case - it's correctly informing people about the invalid file. We could convert the FALSE to NULL though but that's not the only thing the MR does.
Committed/pushed to 11.x and cherry-picked to 10.5.x, thanks!
Back to needs review for the current iteration - I left the test in (see comment above).
@ressa yes it changes out of the box behaviour so I think it could use it.
Committed/pushed to 11.x and cherry-picked to 10.5.x, thanks!
Committed/pushed to 11.x and cherry-picked to 10.5.x, thanks!
Committed/pushed to 11.x and cherry-picked to 10.5.x, thanks!
Committed/pushed to 11.x, thanks!
Committed/pushed to 11.x, thanks!
If I followed the reasoning correctly, that would imply to add a new prop or slot to the navigation:toolbar-button component.
Either that or preparing the 'text' prop before it's passed into the SDC, but we're beyond the limit of my understanding of SDC conventions and limitations so yes someone who's more on top of that would be great to hear from.
This would allow us to completely drop the separate caching if it works. I'm not sure if it's strictly necessary, what's in the MR should be quite efficient as it is, just it would be even more efficient.
When I started looking into the feasibility of this I was wondering about adding the capability generically to menu links, which could be generically useful for some use-cases although probably harder. This is less generically useful but also probably easier. We could also just go ahead more or less with what's here and open another follow-up to explore.
Very minor but I'm wondering if these could be e.g. 'Medium date (without time)', rather than starting with 'no time'.
Since this is user-facing tagging for product manager review.
Looks like this needs another rebase.
The upgrade path looks solid to me and handles all the trickiness around deprecations vs. not.
I can't remember how we translate validation constraint messages, but since we already have messages like that as class properties, I'm assuming we do somewhere?
The body of the function should also trigger a deprecation I think. Unlikely that anything is calling this, but if it was, it would tell them.
Couple of comments on the MR.
I'm wondering if we could make a username-only placeholder work after all.
{% if menu_level == 0 %}
{% if item.below is empty %}
<li id="{{ item_id }}" class="toolbar-block__list-item">
{% include 'navigation:toolbar-button' with {
attributes: item_link_attributes.setAttribute('href', item.url|render|default(null)). setAttribute('data-drupal-tooltip', item.title).setAttribute('data-drupal-tooltip-class', 'admin- toolbar__tooltip'),
icon: item.class|clean_class,
html_tag: item_link_tag,
text: item.title,
modifiers: [
'collapsible',
item_link_tag == 'span' ? 'non-interactive' : null
]|filter(v => v is not null),
} only %}
</li>
{% else %}
<li id="{{ item_id }}" class="toolbar-block__list-item toolbar-popover" data-toolbar- popover>
{% include 'navigation:toolbar-button' with {
action: 'Extend'|t,
attributes: create_attribute({
'aria-expanded': 'false',
'aria-controls': item_id,
'data-toolbar-popover-control': true,
'data-has-safe-triangle': true,
}),
icon: item.class|clean_class,
text: item.title,
modifiers: [
'expand--side',
'collapsible',
],
extra_classes: [
'toolbar-popover__control',
],
When item.below is set, the template extracts the link text for the button.
If we added an additional button_label variable for use only in that branch of the template, could we use that instead? And then that would need to support button_label being a render array so it can include the #lazy_builder etc. Not sure how compatible that is with SDCs but it seems feasible.
Since the custom JS approach is being removed as part of this MR, Drupal\Tests\navigation\FunctionalJavascript\NavigationUserBlockTest could be removed as well, I think.
I think this is probably still useful - makes sure the cache contexts are correct at least.
I think we could placeholder only the username if it wasn't a menu link at all, but we'd lose the fallback menu item logic then. Maybe we could open another follow-up to look at finding a way to do it still, but yeah I think given it's just this block now, caching per user isn't too bad. The navigation itself will still load instantly as soon as there's been a single request for it, the user block will be big-piped in per-user then instant.
Addressed the comments on the MR I think.
I'm not sure the test only job supports nightwatch at all.
It might be worth a second, test only MR, to show the nightwatch test failing without the fix just to make sure it's not a bigger CI issue.
Ready for review now I think.
Cache IDs for the navigation user block look like this:
| navigation_user_block:[languages:language_interface]=en:[theme]=olivero:[user.permissions]=is-admin >
| navigation_user_block:[languages:language_interface]=en:[theme]=olivero:[user]=1
Cache storage requirement shouldn't be much because even with 100 users actively using the navigation that's only 100 unique cache items.
I think this existed in Drupal 7 views but I do not see any use case for it. If a text format is referenced by something else then it will be handled by entity reference formatters now anyway. Moving to won't fix.
OK looks like we do have some existing test coverage because it's failing - didn't check whether it's a bug in the test or the MR yet.
Created an MR from your branch for easier review.
One comment on there but the logic looks good to me otherwise.
I'm not sure how easy this will be to test, or if we even have any test coverage of the remember feature at all - maybe if we could we could extend that.
I think it might be worth trying to move this to the filter base class, but we can do that in a separate issue and concentrate on the bug fix here.
This is all covered by views in core now per #13.
I think hard-coding the permission is fine in this case so #31 looks good to me.
It would be nice if there was a way to use a plugin method as a string for a #lazy_builder callback, but I can't see a way to do that.
We could create an entire new service instead of having the static method maybe and call a method on that.
Tried and failed to use a placeholder for just the name, nowhere to fit the render array into the menu link structure. We could try to make all of that more flexible but not really sure where to start.
But with CachedPlaceholderStrategy we can use the same technique for the whole user menu as we do for the navigation block itself - placeholder it, but also cache it (per user).
When it's in render cache it comes from straight from that without big pipe, which means no flickering at all now. When it's not cached, big pipe will replace it.
Allows the custom js to be removed.
Have a feeling that running class_exists() on every file in the plugin directory means that the trait file is loaded and triggering this deprecation. Will look into it.
That seems very likely. Should we revert here prior to moving the check up, and then open a follow-up for the filtering?
Merged, thank you!
The MR looks good to me I think, --minimal-changes should reduce the amount of things that can go wrong.
I didn't check if we're throwing away the cache tags too or not, but yes if we're not that would happen. And if we're throwing away the cache tags then unless something else adds the user as a cacheable dependency then content won't reflect changes to the username.
This looks reasonable.
📌 Deprecate TestDiscovery test file scanning, use PHPUnit API instead Active will let us drop some of it again.
I'm really not sure about using templates like this. I realise I'm in the minority using vim (and without whatever vim support would take advantage of this, there's probably something somewhere), but don't think I'm the only one. There's also reading docs on api.drupal.org or gitlab which people sometimes do.
Tagging for release highlights, would just be a sentence in the performance improvements section but this is a good step towards making long-running processes easier to manage.
Committed/pushed to 11.x, thanks!
There's already a bug open for not checking the configuration settings, so switching to a task. 🐛 Views checks the session for exposed filters when it shouldn't Active .
Actually these are two different issues just the same code, re-opening.
This was discovered via 📌 Search must be indexed after recipe is applied Active where search indexing via automated cron started a session for no reason, resulting in headers already sent warnings. Bumping to major and tagging as a Drupal CMS release target.
I opened another duplicate of this issue because I couldn't find it and forgot about opening, but that issue is a bit better written up, so closing this one instead 🐛 ViewExecutable should not be responsible for parsing exposed input Active
There's no overlay module to implement this for now.