Thanks, I'll take the MR for a test drive tomorrow if there's time and if all goes well will merge and cut a release.
We did run into this with a content entity type used in a webform option alter hook implementation. There's a max of ~250ish very lightweight content entities loaded by the call, and we started caching them statically in the hook implementation.
To give a sense of scale of the problem, adding that additional static caching reduced memcached sets from 42,700 calls on a cold cache to just over 2000, so it was fairly significant.
Minor grammar fix; removed two repeated space chars.
We've recently ran into a performance bottleneck stemming from a webform_options_alter implementation that a more effective static caching mechanism would have prevented.
I've added a note on performance to more thoroughly explain the context in which these alter hooks are invoked and several strategies that can be used to enhance the performance of implementations.
I think that adding test coverage for the group optimizer is all that's left. Everything else should have decent coverage.
The group optimizer is also the most complicated to unit test.
luke.leber → created an issue.
I'm not exactly following why the -1, -2, etc header name enumeration exists.
The HTTP spec allows for multiple headers with the same name. IMO changing the name of headers like that might be a new drupalism?
So there are still some major issues -- namely a failure to inline an asset seems to corrupt the asset resolver cache...and there doesn't seem to be a way to spy on the CIDs to invalidate in those cases 😭.
Update issue summary with a more generalized proposal.
I'm not sure that a preconnect would help with LCP at all given the module loads the script async. Loading async removes the JS from the critical render path.
The best thing that the Google Tag module can do to help with improving LCP is to inline the first party JS (or add the async or defer attribute) to it.
Preconnecting to the third party that's already loading async won't help anything unless the LCP element is being loaded from tag manager -- which would be an even bigger problem 😅.
Metrics from a homepage run on Olivero. Test repeated 5x with various throttling applied, results averaged.
As you can see, the results of non-throttled "fast and fortunate" traffic yields negligible gains, but as devices and connection speeds decrease, there's a stark improvement.
Here's a similar metric with a single asset remaining in the critical render path (simulated by dropping a hard-coded link to an empty CSS file in <head>...
This leads to the conclusion that it isn't necessarily how much CSS is "critical" or inlined, but rather the mere presence of ANY render-blocking CSS that is the main contributor to LCP timing as device and network quality decreases.
luke.leber → created an issue.
It's super ugly, but the inline_everything branch might be good enough to take rough benchmarks with?
The one thing it has going for it is that the tests that matter most here don't need an optimal back-end setup. The metrics that matter are all in the browser.
I didn't mean to update !10930 :-(...
I'm going to take a stab at an alternative branch here given !10930 is already being used by client sites.
A bit more info on Acquia Cloud Next here.
Individual response headers are indeed limited to 8kb and exceeding this limit results in an immediate white screen of death. ☠️
When splitting things into multiple headers, it appears that Acquia's infrastructure does not fail, but instead collapses the headers into a single one, comma-separating the values.
For example:
Cache-Tags: ...almost 8k of them, abcd
Cache-Tags: dcba, ...the rest
Is transformed into
Cache-Tags: ...almost 8k of them, abce, dcba, ...the rest
I don't know if this behavior is completely kosher with an RFC or not.
Furthermore, the individual header limit on ACN seems to be slightly less than the advertised 8k. Casual testing indicates that things start failing at 8188 bytes. 🤷
Nevermind! It looks like SSR mode is an option!
I'll have to look a bit closer but the star-rating.js library seems to be a bit susceptible to layout shifting.
Ideally, the markup for an element would be server-side rendered so there aren't any DOM reflows on the client side.
UUID+hostname seems agreeable to me. Should be easy enough to improve upon iteratively if needed.
There's no issue with using multiple inputs for a more unique hash if it catches the 80%.
I think that cache collisions here might actually warrant a meta issue because there's just so much complexity involved. If Drupal didn't have the potential for such large headers this would be a lot easier to solve holistically.
Given the response header limitation for Cloudflare may have increased, it's very likely that upstreams will start hitting resource limitations now.
Acquia is now capped at 8kb for their Cloud Next platform.
https://docs.acquia.com/acquia-cloud-platform/known-issues-cloud-next#se...
I think this almost has to be user-configurable (and enforced?) given all hosting is not equal.
luke.leber → created an issue.
I can confirm that some Acquia enterprise environments handle the Client IP restoration as a platform feature.
luke.leber → created an issue. See original summary → .
Manually posting automated tooling output and screenshots of applying patches isn't really a repeatable / trustworthy way to approach this.
Setting back to NW because the drupal.org automated test runner should be the arbiter of success here -- not local development systems.
Remove noisy bot files.
I gave !5 a test drive against vanilla 11.0.x and 11.2.x installs successfully.
Also committed a test-only fix straight to the project update bot only branch.
Given the nature of the change, I think it doesn't preclude me from a self-RTBC.
luke.leber → changed the visibility of the branch 1.0.x to hidden.
luke.leber → changed the visibility of the branch drupal-11-compat-with-tests to hidden.
luke.leber → made their first commit to this issue’s fork.
Seems pretty outdated to me. See d11 issue.
danrod → credited luke.leber → .
Thanks, everything should be set now. The mobile add maintainer experience on drupal.org is a bit odd but I think I got it.
Let me know if you need anything else, thanks!
Hey Daniel, sure thing. If you'd like to step up please open a task issue. Looking at your account history I have no questions off hand.
This module is not in use by my employer currently and I can't spend hours maintaining it anymore.
re: #90 - honestly I think that's alright. Sites are no worse off than they are now and given the age of this issue, IMO it should be landed as-is. It at least prevents the situation from happening to new users even if certain rare(?) tempstores aren't automatically flushed.
Don't let perfect be the enemy of good here. 🙏
I think that the root of this problem is that the original intent of #3377420 was to target the fallback image.
The <img> element is only a fallback when a <picture> tag is wrapped around it. Drupal's responsive image handling wraps a picture tag around markup conditionally. #3377420 also affects images that are not used in a fallback capacity.
Example configuration:
uuid: 70999a45-bb3c-41f7-9886-ba1fe271b26f
langcode: en
status: true
dependencies:
config:
- image.style.blog_featured_media_l
theme:
- wcstudent
id: blog_featured_media
label: 'Blog featured media'
image_style_mappings:
-
image_mapping_type: image_style
image_mapping: blog_featured_media_l
breakpoint_id: wcstudent.default
multiplier: 1x
breakpoint_group: wcstudent
fallback_image_style: '_empty image_'
In this case, the responsive image style only has a single child image style -- which causes Drupal Core to omit the wrapping <picture> tag.
Example markup before #3377420
<img srcset="/sites/default/files/styles/blog_featured_media_l/public/2025-06/need-help-blog.png.webp?itok=HHMwicZj 1x" width="1200" height="900" src="data:image/gif;base64,R0lGODlhAQABAIABAP///wAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==" alt="a colorful note that reads "need help?" " loading="lazy">
After
<img srcset="/sites/default/files/styles/blog_featured_media_l/public/2025-06/need-help-blog.png.webp?itok=ikfUUuon 1x" width="1" height="1" src="data:image/gif;base64,R0lGODlhAQABAIABAP///wAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==" alt="a colorful note that reads "need help?" " loading="lazy">
The result is a pretty significant B/C break that makes the image inaccessible to sighted users.
I think you may be correct in that cache busting will happen automatically if a version is omitted.
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...
Learn something new every day!
Add related issue that caused this.
luke.leber → created an issue.
Setting an official precedent for JIT updates is a great idea, especially given the pending majorly disruptive issues for Layout Builder.
I've done more than a bit of thinking on this and I think that the existing sequential update framework for database updates in general makes for a good basis. If one thinks about it, JIT upgrades are more or less an unrolled version of that, right?
It makes a lot of sense to provide a JIT equivalent that can handle untold millions of records without needing to worry about performance.
Module uninstall requirements can likely handle the more...otherwise prohibitively expensive computations of when it's "safe" for a site to remove a JIT step, yes?
Nowadays ECA is also an option to accomplish this. 🙂
I think someone from the security team should also chime in here given that inlining <style> tags here is inherently more dangerous than loading an external source.
Injecting a </style> somewhere in a CSS asset could lead to a new kind of XSS in core (or otherwise unintentional content injection).
Along side #21, it seems to me that the most easy to use and least prone to error mechanism could be to add a way to flag CSS that is guaranteed to never be critical and load that outside of the critical path.
For example, Drupal off canvas, parts of menus that are only visible on user activation, contextual, skip links, and if custom theme owners are confident enough, anything else they want to flag.
That seems very doable in a follow-up. It might be a good next step to tally up what proportion of core styles are not critical to get a sense of what the long term ROI would be from pursuing #21. That might be a bit time consuming though.
I am mildly confused why this can't be preprocessed back in normally?
/**
* Implements hook_preprocess_HOOK() for form_element__datelist.
*/
function my_theme_preprocess_form_element__datelist(array &$variables) {
foreach (Element::children($variables['element']) as $child) {
$variables['element'][$child]['#title_display'] = 'invisible';
}
}
should work
Hey folks!
I've re-targeted this issue against 2.x-dev. Good news...there's now automated test support.
If someone were to provide a failing test that proves this is an issue it can likely proceed further than it has the past 4 years :-).
Thanks Kyle.
Merged into 2.x and cherry-picked back to 2.0.x. I'll be cutting 2.0.0 stable shortly and will make 2.0.x the recommended version, but will leave 8.x-1.x supported until the end of Drupal 10 support.
Highly doubtful that this module will see another commit until Drupal 12 though.
luke.leber → changed the visibility of the branch project-update-bot-only to hidden.
This is NR (for real) now for !4. Green pipelines and adjusted target branch.
@kyle | @brianne | @zach - any chance of a review for this? Thanks.
luke.leber → made their first commit to this issue’s fork.
Thanks, I plan to open a pull request with an automated test and the appropriate gitlab CI configuration tomorrow and get someone from our internal team to review it.
In all honesty, I haven't had any sponsored time to spend on XB since Atlanta.
I would be curious to hear what Pierre thinks since his SDC advice of iterating slots (versus pursuing dynamic slots is what led me to discover the hacky workaround.
It does seem a very, very fragile state of affairs that iterating a slot's components and injecting some markup can cause XB's front-end to fail by using only well supported twig. I'm assuming that a simple preprocess could do the same.
I do think that this quirk will be fairly commonplace to find in the wild.
smustgrave → credited luke.leber → .
So to summarize the immediate problem to move this forward, when trying to determine if something is an image, there's a string input that's basically in a random format. It might be a scheme://path.extesion?query#fragment with any part present, not present, or even malformed. Anticipate that the string might even be something like a string of random emojis and some unprintable / null control characters, or even characters that aren't allowed in a URL at all that may or may not be encoded - Some of these methods seem to be general utility methods which may be called outside of Mime Mail given that the utility class is not marked @internal!.
This string has to be either:
- Massaged into something that the mime type guesser won't choke on
- Assumed to probably not be an image if things are too messed up to pass to the guesser
Even given Drupal's URL utilities, that's likely extremely tricky as this issue stands. IMO it might be worth checking the various call sites and seeing if there's an opportunity to normalize things. That would of course likely need a B/C break in the utility class, but I do fear that tackling the problem naively may be a very disturbing game of whack-a-mole in the wild. Passing a nullable \Drupal\Core\Url object into the various utility methods would make this a trivial problem to solve.
luke.leber → created an issue.
luke.leber → made their first commit to this issue’s fork.
luke.leber → made their first commit to this issue’s fork.
luke.leber → created an issue.
This change will need a post-update hook to re-save existing configuration.
There's another issue at play here as well. With the syslog module enabled, calls to drupal_flush_all_caches resolves the asset.js.collection_optimizer service, which resolves about 5 other services until it eventually gets to creating a Syslog object. This object causes a cache_config rebuild before drupal_flush_all_caches is ready to start rebuilding.
Specifically, the drupal_static and to a lesser extent the twig caches will be stale / corrupt when this rebuild happens. The state of Drupal is kind of in a weird half-flushed state where all of the bins are empty, but drupal_static isn't.
Crazy idea...but does it really matter if the tempstore entity is updated if the entity was saved with LB? Seems to me at face value that's a premature optimization. Can the route check just be removed...?
2) For now the expectation is that more complex use cases (like tabs or sliders) will have to be coded for by the component developer making use of that window.isInsideExperienceBuilder
I think this could work for the most part if there was sufficient render-time context to conditionally render additional JS. From the limited understanding I have of the semi-decoupled theme engine, that seems possible, so 🥳.
I apologize that I no longer have any officially sponsored time (9-5) to delegate to this effort, but it was a tenuous arrangement from the start. I'll endeavor to respond to issues I'm already involved in until closed though on my own time.
I would like to draw some attention back to the gif at the end of #7. While a naive "in_preview" works for simple accordion-like controls with Boolean states, things like tabs, which could have multiple mutually exclusive states remains a question mark.
To accomplish this in layout builder, our team currently injects additional JS on top of what normally runs on the front-end component to keep track of state.
Admittedly it is a bit clunky as there are massive layout shifts on re-paint though...so I wouldn't read too much into how we "solved" it with layout builder.
Yep! That should do it. I'm not 100% positive on the rationality behind it, but it's a very common pattern for when changes to a module (or core) require a cache flush.
If I had to guess it's so that site owners will see that database updates are required in their dashboards, and when they run said updates, the caches naturally flush out.
Hey,
The call to Html::getUniqueId should prevent duplicate IDs 🤔.
That's mainly why I'm interested in how this is actually happening. I wasn't able to reproduce this. The only thing I keep coming back to is if perhaps your theme or module is also using that ID without wrapping it in Html::getUniqueId?
If the uniquification is broken, we would have to fix it. Removing that ID would be a backwards compatibility breakage that can't happen until a new major is released.
It's my understanding that the mere presence of a post-update hook is sufficient to ensure that caches are flushed appropriately. I don't think we need to call the cache flushing function manually inside.
Almost there for 1.x🙂.
Regarding the 2.x branch and tests, it looks like Drupal 11 has indeed broken some functionality that will need to be addressed 😞.
Best to stick with the stable release until that bag of worms can be resolved.
I gave this a manual test (via the attached model) and everything still seems to be in order. Switching back to NR - thanks Jürgen!
luke.leber → created an issue.
I think the concept is useful, but sometimes the customization can't be hard-coded as data attributes, but rather rely on runtime information.
One example is for the international telephone element. See https://git.drupalcode.org/project/webform_intl_tel_national_mode/-/blob... for a somewhat heavy handed fix for how most UX problems were resolved.
Were there a client side event to tap into, it'd make things a lot easier to tweak. I would LOVE to take a holistic pass on the telephone input given how many quirks there are, and if there was more robust options handling, that'd be even easier.
Moving back to needs work due to...
- The issue summary describing the problem but not clearing laying out a proposed resolution.
- This issue has a visible patch (that doesn't apply to HEAD) -- patch workflow is antiquated and will not run automated tests anymore.
- This issue having 2 forks (which are hidden by default) -- both of which go different directions than the patch.
All in all, I don't think this can be "needs review". There needs to be a written-down plan and one up to date merge request to be eligible for needing review IMHO.
That all said something definitely needs to be done here. The tap area for the current out of the box experience is too small to pass accessibility audits, so tagging with accessibility.
Back to NR for !11689 (10.5.x) - I'm out of time to open other PR's, we've decided to patch this in as needed.
luke.leber → changed the visibility of the branch 3471642-upgrade-asm89stack-cors-to to hidden.