That was 🐛 Navigation Top Bar hides entity local tasks even if the user has no access to the bar Active , should be fixed now hopefully.
So what happened is we added the new performance test, the last MR run here was before that commit, so we didn't get a test run with the new test. Just one extra cache get, probably from the permissions check. Going ahead and committing here to unbreak HEAD.
This broke head, opening a follow-up MR to fix the test.
catch → made their first commit to this issue’s fork.
Opened 🐛 RouteNormalizerRequestSubscriber redirects away from the front page when it's set to an alias Active too for the redirect issue.
catch → created an issue.
Your backtrace shows a cache key with context_stack, which looks like it's from https://www.drupal.org/project/context_stack → which has a large notice on its homepage recommending to uninstall it.
Can you try uninstalling that module and see if the problem persists?
Re-reading the change from [##3457863] it seems like something that used to silently fail before now fails loudly. Can't really see how that issue would have introduced a bug as such.
I think we should commit a quick fix to go back to silent (or logged) failure here, but it would be good to understand how sites are running into this issue in the first place.
If this gets into 11.2 we can backport through to earlier branches, but adding the tag for additional attention.
package_manager, which is the underlying API used by both automatic_updates and project browser, is now in core as an alpha experimental module.
Once it reaches stable, automatic updates and project browser will be able to depend on the core code directly which will make the contrib surface area smaller.
We're also talking about the best way to get automatic updates and project browser into core - however now that both are included in Drupal CMS, they'll be getting a lot more exposure/testing soon anyway.
Yeah I think showing nothing for recipes is better than hard-coding, and then we can try to figure out what it actually should show.
Agreed with #10 that the policy could change so it would be better to hardcode it. However I also think doing a quick fix to match the current policy might be worth doing anyway, because the final fix seems non-trivial to me.
Is there an issue to switch to the navigation top bar? Currently sites using gin_toolbar + navigation (e.g. all Drupal CMS sites) load about 300kb of JavaScript for admin users, and a non-trivial amount of this is from the old toolbar module - I didn't count how much but it looks like somewhere to 50-100kb.
Green!
Content editors still have a very large amount of JavaScript, but it's not from Klaro, it appears to be mostly because both navigation and toolbar are loaded at the same time, which I think is a Gin issue?
For example, Search API ships with a 'Rendered entity' processor plugin that lets you configure a view mode to use for each entity-type bundle. During indexing this plugin renders the entity and the results can be used in a search API index. This is typically combined with something like a strip HTML processor and one that weights e.g. h2/h3 higher. This is however not very precise, if you have a common footer component on every page those elements end up in the index for every page.
The proposed solution should provide a list of enabled component config objects and a checkbox for each of their props.
For example, you might have a card component with fields for image, title and teaser text.
It's not clear to me how this would interact with non-XB field data on the entity (title, intro, hero image etc.)
Let's assume that those entity-level fields are configured within an XB layout, then does the processor also handle how those are rendered for search indexing, or do you need to set up both a search view mode and this processor side by side - one for the entity-level fields, and one for the XB props?
Postponing this on 📌 Implement caching mechanism Active and 📌 Get coffee data only when the search box is opened Active - those can be done without complex complex cache invalidation logic (or cache staleness bugs), and should make this less necessary. If it's still necessary after those, then dynamic_page_cache support would allow for a short TTL here without much impact on server load.
A way to verify that this tests the specific bug in 🐛 Block visibility settings have summary duplicated in the title Active would be to add an additional branch here, revert the commit from that issue, and open a draft MR - then we should be able to see the new test coverage fail.
@nikolay shapovalov it's possible that's a random failure but this is the first time I've seen it reported anywhere, just re-ran the test over there. If it happens again, we should make the assertion between 60 and 61 then open a follow-up to try to track down what's changing.
Splitting into two seems like a good idea. If Ludwig doesn't use any of the routes etc. , I think we could still remove those, and those are the confusing parts.
There might be at least three bugs here:
1. Redirect module doesn't check for '/' before redirecting away from the front page, but if anything it should redirect /home back to the front page. It could also save all the lookup logic in RouteNormalizerRequestSubscriber if it short-circuited on '/'.
2. PathMatcher::isFrontPage() only checks the configured string of the front page path, not the URL it resolves to etc. This might be 'by design'.
3. Core should have a config listener that resolves aliases to system paths (where they exist) instead of the logic happening in the form.
Both #1 and #3 seem like potentially quick fixes that could go into patch releases fwiw.
Responding to some comments on 🐛 Security Advisory coverage status is hardcoded on Recipes Active .
@pameela
If you were starting a new project today that had to use webform, would you really use D10 so that you had security coverage?
I would use the alpha, but there are mitigating factors:
1. I've got 20 years of Drupal experience and know how to apply patches and debug things if they go wrong.
2. If I start a project now and use an alpha release, there is a non-zero chance the project will have a release candidate or stable release by the time that project launches.
3. I would rather spend time helping get modules stable on a new major release, than deal with a major core update 6 months later. Not necessarily because it's less work, but because it feels like a choice between spending time usefully on things that benefit all Drupal websites rather than busywork within an individual project.
For the same reasons, I have existing Drupal 10 sites that I probably won't start updating to 11.x until mid-2025 in the hope that the total amount of hours/budget spent on updates will be lower in mid-2025 than it would be now (or in 2026 when Drupal 10 is nearly out of support and contrib has moved on). e.g. if they have 50 modules installed and 30 of them are alpha now but will be stable in May 2025, that's a lot less to worry about.
However Drupal CMS is primarily aimed at non-technical Drupal users, and my understanding of it is it's specifically supposed to remove a lot of those considerations when building a site.
@poker10
But Drupal CMS/Forms recipe/PB will make the decision (without acknowledgment) instead of users, which is the difference.
Yes this is the difference for me too.
The webform recipe isn't enabled by default, so it would be entirely possible to remove it from Drupal CMS today.
If project browser can install recipes, then individual sites can make the decision to install the recipe, or webform directly, but if Drupal CMS's minimum-stability is stable, then project browser should prevent that from happening.
But then site owners have workarounds they can make (like lowering minimum stability themselves), or as soon as there's a stable tagged release, it would work.
grep -rl "webform"
drupal_cms_forms/tests/e2e/contact_form.cy.js
drupal_cms_forms/recipe.yml
drupal_cms_forms/content/shortcut/create-form.yml
drupal_cms_forms/config/captcha.captcha_point.webform_submission_contact_form_add_form.yml
drupal_cms_forms/composer.json
for example the Forms recipe in Drupal CMS still contains Webform 6.3.0-alpha3, which means that you will end up with a not SA covered module(s) in specific cases
I think this might be a general issue with project browser and dependencies of projects that are going to be installed, rather than just recipes.
e.g. if I install https://www.drupal.org/project/webform_booking → that has stable 11.x support via project browser, this project depends on webform which is alpha.
Unless project browser recursively calculates the dependencies, and shows me all the things that are going to be installed and which version, then installing a stable, security supported project doesn't guarantee this across its dependencies.
I've replied to @pameela and @poker10's other points on #3447882-14: Determine some heuristics for module inclusion to avoid conflicts with core functionality, set expectations for security coverage etc. → because we're at risk of going off-topic here.
@pameela
If you were starting a new project today that had to use webform, would you really use D10 so that you had security coverage?
I would use the alpha, but there are mitigating factors:
1. I've got 20 years of Drupal experience and know how to apply patches and debug things if they go wrong.
2. If I start a project now and use an alpha release, there is a non-zero chance the project will have a release candidate or stable release by the time that project launches.
3. I would rather spend time helping get modules stable on a new major release, than deal with a major core update 6 months later. Not necessarily because it's less work, but because it feels like a choice between spending time usefully on things that benefit all Drupal websites rather than busywork within an individual project.
For the same reasons, I have existing Drupal 10 sites that I probably won't start updating to 11.x until mid-2025 in the hope that the total amount of hours/budget spent on updates will be lower in mid-2025 than it would be now (or in 2026 when Drupal 10 is nearly out of support and contrib has moved on). e.g. if they have 50 modules installed and 30 of them are alpha now but will be stable in May 2025, that's a lot less to worry about.
However Drupal CMS is primarily aimed at non-technical Drupal users, and my understanding of it is it's specifically supposed to remove a lot of those considerations when building a site.
@poker10
But Drupal CMS/Forms recipe/PB will make the decision (without acknowledgment) instead of users, which is the difference.
Yes this is the difference for me too.
The webform recipe isn't enabled by default, so it would be entirely possible to remove it from Drupal CMS today.
If project browser can install recipes, then individual sites can make the decision to install the recipe, or webform directly, but if Drupal CMS's minimum-stability is stable, then project browser should prevent that from happening.
But then site owners have workarounds they can make (like lowering minimum stability themselves), or as soon as there's a stable tagged release, it would work.
Yeah I agree that a good first step would be to show nothing instead of misleading information.
What happens now in project browser if a module is opted into security coverage, but it depends on a module that is not opted into security coverage? Does project browser expand the dependency tree before installing dependencies or not? If it doesn't then this is probably applies to modules and themes too.
And is the badge purely related to the opt in status or does it also check for stable releases? Projects can and do opt into security coverage when they only have alphas, just means the coverage actually starts at stable. So if we need to conditionally show the badge this comes back to the other related issues here about which version will get installed.
I think if we're removing the entire capability then whether we deprecated one API method or not already is not going to matter.
So the one question here is should we make an exception to the BC/continuous upgrade policy to remove this wholesale in 11.2, on the basis that we think noone uses it, it probably doesn't work, and that it directly conflicts with features in Drupal CMS.
For me the answer to that is yes but will ping other committers for more +1s.
Gitlab is not especially happy but this appears to be pre-existing.
catch → created an issue.
#16 is worth a try and it would be easier to add a test. Since it could potentially run on every request, we should probably do it in an assert rather than actual logging?
Probably need to support include files too but that can be done in ModuleHandler::loadInclude().
Bumping to major because this is potentially a big Content Layout Shift improvement for all big pipe responses, it will also work very nicely with the cold cache optimisations that 🌱 Adopt the Revolt event loop for async task orchestration Active will enable.
Discussed the above a bit more with @plopsec and swapped out 📌 Implement a caching strategy for the menu links Active for 📌 Render the navigation toolbar in a placeholder Active in the issue summary.
Discussed with @plopsec and I'm moving the navigation stable blocker tag to this issue from 📌 Implement a caching strategy for the menu links Active .
📌 Add render caching for the navigation render array Active just landed which unblocks this issue.
We might need to split this issue into separate 'add the API' and 'use a placeholder' issues, but since the end goal of this issue is that there's zero impact on the front end, except for a render cache miss served by BigPipe, not sure how to test all of this yet, so will be nice to be able to piggy-back off the existing test coverage and manually test etc.
📌 Add render caching for the navigation render array Active landed which unblocks 📌 Render the navigation toolbar in a placeholder Active .
Marking this as PP-2 and added that issue to the issue summary here. I'll swap the navigation stable blocker tags over too. If we have to change direction again we can switch things around again, but pretty sure this will be both easier and better than adapting the toolbar tree caching implementation so let's try it!
Before the latest version of the MR I made an attempt to skip the entire hunk if libraries were empty, but it failed and it's reasonable for modules to alter things when the list is empty etc., so yeah I think current state here is as far as we should go.
I don't have any good ideas to test this yet - we could probably write something that tests the implementation, but we could have done that before the change here and the test would have been wrong.
I think we could probably backport this to 11.1.x given that navigation is experimental and there's no bc implications here.
That exception is thrown only when another process is holding the lock on creating the image which should be almost impossible with a single user site.
Next step would be to add some debugging/logging in ImageStyleController where the lock is about to be required, and log a backtrace.
There's no image style pregeneration on cron like image_style_warmer is there? That could cause this, and generally is unlikely to work well on Drupal CMS sites because there's a high likelihood of them relying on automated cron module.
If it's not pregeneration, maybe something safari-specific is causing the image to be requested twice by the same page request?
We could look at adding some lock-wait-and-retry logic to ImageStyleController to make it less keen to send a 503, but should track down how this condition is getting reached at all first.
larowlan → credited catch → .
It might be possible to skip the cache get/set still, I had to give up trying to skip that to get tests green, but now they're green we could try again.
The main barrier is the $settings_in_header variable which is set based on whether any libraries requested are in the header and also depend on settings, but if no libraries are requested, that is FALSE. So potentially accounting for that, and moving some variable creation back around, might be enough.
Project Browser can install anything it wants regardless of the preferences in composer.json if it makes calls like $installer->require(['drupal/webform:@alpha']);
To do that, would it not need to know that webform:@beta didn't work first? I guess it could try with gradually decreasing levels of stability though?
I personally would not be too worried about making LCP of a youtube video worse, because it's already bad, and using something like https://www.drupal.org/project/lite_youtube_embed → would make it much better, and mean that third party js would only be loaded when it's interacted with, not when the page is loaded.
No idea how that module interacts with Klaro though, if it blocks the lite youtube render too, then that'd be trickier.
Also all the pages without a youtube embed would be faster anyway.
The most likely cause of this is that the first front page visit is creating the image style, this means the image is served via PHP, and then on safari specifically, something is wrong with the headers.
I don't have safari to test with, but someone who can reproduce this could check the following:
1. Are there any console messages from the first load of the front page - mis-matched mime type etc.
2. If you view the network tab (or safari equivalent I guess) when viewing the front page, what do you get for the image?
There is 📌 Update performance tests when Klaro tags a new release Postponed open which updates the performance test coverage - it shows that out of the box Drupal CMS will have 0 js or css from Klaro loaded now which is great.
What we don't have tests for is the impact once an app is enabled, that would be good to add especially once 🐛 Klaro library seems way too large? Active is in a tagged release.
I think this issue can probably be marked fixed/duplicate once the above issues are all in a tagged release. Really nice to see this come together.
Does the Klaro Drupal js need to be set to preprocess: false and also set to defer to match? Then the browser should still load the klaro library first, which would mean all the js ought to continue to work then.
Yes it's better.
ComposerPatchesValidator is still 160s on that run, but this is less than various other tests. Just kicked off a run and none of the package_manager tests were the slowest on that run.
I think we might be better off doing 📌 Render the navigation toolbar in a placeholder Active (see some of the discussion in that issue).
If we do that, then navigation needs to use a #lazy_builder for the navigation render array and core will handle efficiently rendering it via render caching / dynamic_page_cache / bigpipe depending on what's enabled. The HTML will be served directly, no need for local storage and hence no need to worry about the flickering issues that affected the toolbar module or custom js etc.
This needs one improvement to core's placeholder rendering strategy (the CachedPlaceholderStrategy sketched out in that issue), but that improvement would benefit any situation where placeholders are used, not just Navigation.
Not closing this as a duplicate yet but if it all works, then we'll be able to keep things simple in Navigation itself, and also improve overall core page serving performance at the same time.
So to make this work:
Add a CachedPlaceholderStrategy - this does a multiple get on render cache items, replaces the placeholder directly with the cached HTML.
This runs all the time, before big pipe's own strategy (if big pipe is enabled). Big pipe then will only deal with placeholders which aren't already render cache hits.
When you have several placeholders render cached for stuff within the viewport, this will significantly reduce content layout shift and maybe largest contentful paint too, because all the HTML gets sent together until it can't be.
It will also work very nicely in tandem with 🌱 Adopt the Revolt event loop for async task orchestration Active because then anything expensive in the uncached placeholders that can be done async will be.
Talking to Fabianx:
CachedPlaceholderStrategy
c) BigPipe in particular would be combined with a CachedPlaceholderStrategy. Compared to single flush, that one could make use of $renderCache->getMultiple() (as soon as that exists), because again something that is cached already does not need to be BigPipe’d.
placeholder_strategy.cache: class: Drupal\Core\Render\Placeholder\CachedPlaceholderStrategy tags: - { name: placeholder_strategy, priority: -900 }With the code to first generate the CIDs, then do a $cache->getMultiple() and return the HTML for those placeholders that had a cache hit.
With 📌 Implement a caching strategy for the menu links Active RTBC, I am starting to think we could skip doing 📌 Implement a caching strategy for the menu links Active , especially if we can figure out 📌 Render the navigation toolbar in a placeholder Active . We probably want to check exactly how much HTML we're sending to the browser, but otherwise those two issues should speed things up considerably.
Another thought.
For dynamic_page_cache efficiency, we would want this in a placeholder.
But to avoid cumulative layout shift, we might not want it to be rendered via BigPipe.
BigPipe already has non-js placeholders, for placeholders within attributes etc. which are blocking and sent with the main response. Maybe we can add some kind of additional hint for the placeholder strategy so that the navigation placeholder always gets rendered via a nojs placeholder (i.e. inline with the main response) - this would address both caching efficiency and layout shift then.
catch → changed the visibility of the branch 3493406-test-coverage to hidden.
Yeah I think we could probably detect when something will definitely not get installed, but not when it might be possible to install but its own dependencies cause it to be not-installable, especially given some of those could be non-Drupal projects from packagist etc.
I imagine a power user would set minimum-stability to their comfort level and users using PB would just click install, not caring about module versions.
I don't think this can be relied on.
For example Gin has never had a stable release, and it's quite likely that someone would want to install Gin with project_browser. If their minimum_stability is stable, they won't be able to do that, so they'll be searching how to install a release candidate.
Overall I think it's a good thing if there's some friction involved in that.
But assuming they figure out how to change their minimum stability globally, the next project they install could also be in permanent beta/rc and project browser will happily install it without them even realising it's not stable.
The problem is that the output isn't really human-readable (and I'm not sure if it can be made machine-readable), and if Composer doesn't leave behind any artifacts for us to parse (for example, the lock file that would result from a real run), then I'm not sure we have anything we can produce a user-facing summary from.
This makes sense.
If we got the release data from the d.o API instead (which I assume project browser actually uses), could we then cross-reference this against what the site has set for composer minimum stability?
This would also potentially pre-warn people when they're not going to be able to install the project then - not sure what the messaging looks like when project browser fails to install a project.
Yes this is the problem - if people install from composer templates that set minimum stability to something other than stable, which will be the case for any pre-release of a distribution like Drupal CMS (and maybe stable releases if the stability doesn't get raised before the release day), then they'll be defaulted to alpha or beta or rc without knowing about it. This might not be project browser's problem as such but the combination of the two seems very bad.
Not sure about an event here, tagged services might be easier to follow - left a comment on the MR.
The schema change and making this flexible seems good though.
OK I made some more progress. I think this was probably broken when we added caching in the first place in 2015, which I worked on...
The main problem is that the decision whether to add settings depends on whether any library that is already loaded or about to be loaded adds the drupalSettings library as a dependency. If it doesn't, we want to ignore all the settings added, because there's nothing to process them. We do it like this because various settings get added 'just in case' even if nothing will use them.
However the already loaded libraries are not included in the cache key (and shouldn't be) so that logic is flawed.
What I've done is move that logic out of the cached section entirely, but tried to keep the same overall conditions. I made various mistakes and these resulted in test failures, especially in Ajax\FrameworkTest, so we do have pretty good coverage to show that this isn't breaking some things that already work, but obviously no coverage of the bug itself yet.
AssetResolver does have some unit test coverage in AssetResolverTest, so maybe we can add some coverage for this issue without having to do all of #2614936: Improve unit test coverage for AssetResolver → . Or maybe we can extend FrameworkTest to account for this case too. Or both.
This looks obvious now reading the diff I ended up with, it was not obvious before actually making the change exactly what logic can be cached and what can't. I'm pretty sure we actually cache what can be cached and don't cache what can't now. A bonus is this should result in better cache hit rates too.
Test run is green except for one random failure.
There are some lines moved around here that don't need to be with the final result, might move them back where they were to reduce the diff.
Bumping this to major, while it's technically a workaround for extra whitespace in PHP code, it would make everyone's lives a lot easier if it works well.
fwiw agreed with the CSS fix here. There are three issues blocking the navigation top bar from being stable (more for navigation itself) listed on 📌 New Toolbar Roadmap: Path to Beta & Stable Active , so it may be possible to switch to the navigation toolbar in Gin / Drupal CMS when 11.2 comes out, or earlier if some of those are backportable to 11.1.
Went ahead and made a pre-emptive MR here - just updated Klaro to the latest dev locally and ran the tests to get the new numbers.
That makes sense with the Nightwatch mis-match, good to narrow everything down. Let's get this in.
Good find.
First of all I was going to say we should hash the settings into the cache key, but the settings logic isn't cached. I've pushed a second branch which attempts to do zero libraries work if there are no libraries, but also doesn't cache anything. Didn't run tests on that yet, approach could be completely flawed.
catch → made their first commit to this issue’s fork.
All green on https://git.drupalcode.org/issue/drupal_cms-3491405/-/pipelines/370852 - did you rerun the failing jobs or something?
If we move the performance tests to their own job it will then be easier from there to add a job to populate the Grafana dashboard, so +1 to that. Opened 📌 Run performance tests in their own job Active .
Right but presumably UX and features would include things like responsive image and resizing support, CSS and JavaScript aggregation or the lack of it?
And then I was hoping this could be expanded to page weight / loading times.
Isn't this only a testing issue? Moving to phpunit but if there's more to it, could use an issue summary update.
I think we can ignore the warnings, they might be pre-existing. Should open a follow up to try to clean this up though.
11.x is currently the same as 11.2.x and we'll branch off that - 'next minor' should point to 11.x until 11.2.x is available, which won't be for months.
Lots of merge conflicts in the rebase again, so just went for the merge commit this time to save time.
🐛 Privacy policy link goes to a 403 Active will probably start to fail when this is merged, but only in a good way in that things are improving.
Where this this benchmarking being tracked?
Came here to ask if any performance comparisons are being done - e.g. if people are setting up local ddev installs (or default hosted installs for SASS) then it would be possible to run lighthouse or similar against the install and compare vs. the default install of Drupal CMS. It could use some standard steps (e.g. testing logged out vs. as an admin etc.).
I have an idea. core.services.yml contains the renderer.config container parameter. It contains required_cache_contexts: ['languages:language_interface', 'theme', 'user.permissions']. If we would generalize that to all cache bins
We can do this for caches that implement VariationCache, but it can only be at level, not the cache backend implementation itself. So we'd still need to make a load of caches VariationCache enabled - but these are the same sorts of things that already vary by language and similar, so it would be a good change in general, probably.
I think we can probably backport this to 10.3.x too.
Another possible approach to config overrides would be using VariationCache for a lot more things, and then adding default cache contexts similar to the render cache default cache contexts, which workspaces could then add itself to. This would be a lot of work though.
One thing that makes me very unsure about workspaces and configuration is the possibility that the configuration change when you publish a workspace will affect more than just the content in the workspace being being published and the same goes for a revert.
I think for publishing this can be covered by some extra messaging when a workspace includes configuration changes. Anything that allows for staging of config changes on production is going to have similar issues even if it wasn't using workspaces, and the need for publishing content changes and config changes together (a new view that shows some new content etc.) wouldn't really go away even if it was implemented as linking two separate operations together.
Revert is very different though due to config/content dependencies - but again that problem exists with any kind of config revert on production after other changes have been made.
if a site owner decides not to bother about privacy
I think if they did, they would just uninstall Klaro altogether?
#6 seems like a possible test coverage shortcut - could be done with hook_system_info_alter() in a test module.
Thanks I knew I was doing something wrong but couldn't remember what the right thing was.
I think it would be worth implementing object caching for the coffee AJAX callback, after working on 📌 Get coffee data only when the search box is opened Active . This could and should happen independently of local storage because the callback should be cacheable by permissions 99% of the time.
Haven't looked at implementation yet, but it might work if the AJAX request uses GET and the callback returns a cacheable response object, then potentially dynamic_page_cache could handle it with the correct cache contexts and cache tags. If there's something missing, then using VariationCache is possible in core now.
Opened 📌 Get coffee data only when the search box is opened Active .
@poker10 ah sorry yes that wasn't obvious to me but probably would be obvious to someone who's used SEO checklist more than zero times.
Either way installing an extra xmlsitemap module or advagg could hose someone's site in some circumstances (although advagg will probably never have an 12.x release) so agree that bad advice is worse than no advice.
Bumping to major.
@jurgenhaas yes we can adjust the performance tests once this is in. Or if the performance tests land first, tweak them here to show the improvement. But I until one lands no need to worry about the other yet.
It's a long time since I've used shared hosting but the one use of the zip I can think of would be 'upload all the files you need to the web/ folder of a shared hosting account'. But that only works if the shared hosting account has composer available for automatic updates, otherwise that user won't be able to ever update that site.
But even if it's useful for that, it would make sense to provide the zip only once this has been proven to work on a handful of major shared hosting providers, which means removing it until then.