Most of what I was hoping to improve here is covered by 🌱 [Policy] Branch Naming: Use an 11.x branch for HEAD, then use 'main' when d.o can support it Fixed .
Marking as outdated.
The two changes since this was opened are we have the 11.x 'fake main' branch open, which always gets the first commit, then release branches for minor branches, and that we keep the previous major branch open longer.
The 11.x/main branch never changes as a target (except maybe once more when we change to actually use 'main'). This massively reduces churn in MR targets and makes long running issues (anything longer than four months) much less work to manage.
There are a _lot_ of commits that don't cleanly cherry pick even from e.g. 11.x to 11.1.x and this isn't because the bugs change lots of code but because other issues have - test refactors, OOP hook conversions, coding standards changes etc. and additionally there are a lot of considerations about which issues get backported or not - bug fixes with API changes or upgrade paths generally won't get backported at all.
Expecting people to figure which branch to target for the initial MR is unreasonable IMO, it is almost impossible to tell when starting work on a bugfix whether it will land in 10.4.x, 10.5.x, 11.1.x or 11.x and having to maintain multiple MRs or change the target around constantly would add loads of work.
So I think this should be won't fix.
The path module doesn't do anything with index.php so moving to base system.
Pretty sure this logic hasn't changed. There is an entity iterator issue around which would help with this, but a one off with a @todo for that issue also seems fine.
We could likely get to the EventLoop at root by just putting it in the index.php. However, that means everything that deals with Drupal needs to run EventLoop::run itself.
If the event loop is in index.php, does that mean that e.g. drush is required to run it and will break without it, or does it mean that drush would need to implement it to support the event loop? If it's the latter, that seems fine to get things going?
Alternatively if putting it in the kernel enables it everywhere, it feels like that would then enable us to move it to Symfony runtime transparently later.
@poker10 I am fairly sure that PHP installation support was widespread when webp was added to core. imagewebp from the gd library has been available since PHP 5.4 according to https://www.php.net/manual/en/function.imagewebp.php so theoretically that means it was everywhere.
However there was also an issue that older versions of osx, from around 2015 and earlier would not render webp, regardless of browser. This delayed us enabling webp by default in image styles in core until some time during Drupal 10. So we had it available as an option for a long time before then.
With AVIF it has been somewhat the opposite, in that all the browsers we care about supported it before php did. So once we do this issue, we should be able to convert the default image styles in core and Drupal CMS over, give or take the size considerations in the follow-up.
Re-titling, because pre-selected profiles (pre-filled settings.php, distributions), will still be supported. The only thing we'd be removing here is the form/step for install profile selection.
Added a couple of child issues.
@mediameriquat are you able to check the version of composer installed on your hosting? I think there's another issue somewhere where the message when composer is too outdated is not very explanatory.
Agreed with #10. Also if we'd noticed this quickly we might have reverted the original issue and we don't add new year coverage with a revert.
I think this could use a comment as to why we do two queries here so that no-one tries to 'optimize' it back to one query.
Opened 📌 Clean up Claro's tables/tablesort/tableselect CSS Active .
OK the problem is that when the tablesort CSS is moved to the library, Claro's classy tablesort.css, which specifies a td.is-active background color, is loaded after Claro's tables.css, which specifies background: none;
Adding an MR that just removes the declaration from classy.css - it makes no sense for Claro to define two different CSS rules for the same class, the classy one was just never getting used until the regression.
More generally, Claro has tablesort.css and tables.css, but defines various styles for tablesort inside tables.css. So it would be worth a follow-up to move all of that styling to tablesort.css to avoid unused CSS when regular, unsortable, tables are rendered.
Working on it, MR shortly.
@joseph olstad, I don't know why you would suggest rewriting Drupal's wysiwyg integration based on a completely hypothetical ckeditor 6 that is to my knowledge discussed nowhere else on the internet, in a core strategy discussion for 2025-2028. There will not be a ckeditor 6 in 2028, it won't even have been started. There are concrete, known, problems to deal with. Please stop posting off-topic comments on this issue, as with other recent discussions your behaviour is approaching abuse of the issue queue at this point.
@jonathanshaw for render arrays see some of the recent discussion in #2702061-106: Unify & simplify render & theme system: component-based rendering (enables pattern library, style guides, interface previews, client-side re-rendering) → , and other issues like #1804614: [meta] Consolidate similar twig templates and properly use theme suggestions in core → linked from 🌱 [meta] Drupal modernization Active . Some of this needs fleshing out more, some of it is actively in progress like 📌 Compatibility between SDC and the Form API Active . We should improve the render API because it will benefit humans who need to interact with that API, and the sites built on them. 'AI assisted development' is fundamentally developer tooling, and I feel like optimizing for that rather than humans would be a serious mistake - humans still need to read the code that the AI assistant generates after all. We do add things to core for PHPStan etc., but this isn't done at the expense of readability for humans etc. and entire APIs haven't been rewritten just to make PHPStan rules a bit easier and if we put that we were going to do that in a core strategy document, I think people would really wonder what our priorities are.
For Drupal CMS it means things like:
2. Adding modules like ECA or twig formatters to massively enhance the range of custom features that can be delivered safely by configuration alone.
Drupal CMS has already added ECA module. It's been doing amazing things in 📌 Create recipe to clone entities with ECA Active and 🐛 Privacy policy link goes to a 403 Active - I have not actually used it directly yet, but it seems like an absolutely perfect match for the recipe system, that will massively cut down on glue code and in some cases entire contrib modules. But again, it's great in its own right, not just because AI agents can write ECAs in production environments.
Committed/pushed to 11.x, thanks!
Committed/pushed to 11.x, thanks!
Apparently PHP blows up with a fatal error if they're typehinted. See the results of https://git.drupalcode.org/issue/drupal-1578832/-/pipelines/476013. I don't really get why.
Oh I get it now - CacheCollector doesn't type hint the class, so they can't be constructor property promoted and also type hinted.
In this case what we need to do is remove the constructor property promotion, and add back the type hints.
Needs work for that but I think this is ready otherwise.
Oh good spot in #52, that's definitely the old toolbar, completely missed that given shortcuts, user etc. also exist in the new navigation.
Looks good. Committed/pushed to 11.x, 11.1.x and 10.5.x, thanks!
Procedural hooks haven't been deprecated yet. Similarly we haven't deprecated annotation discovery in 📌 Triggering deprecations for plugins using annotations when core plugin type has been converted to attributes Needs work either, I don't think either of those will happen for 11.2, so the earliest they could land is 11.3.
However, although I can't immediately find it, we'd already discussed deferring both of those deprecations for removal in Drupal 13 anyway.
For annotations, there are all of the plugin annotations to convert, but also all the plugin managers need to support attributes before that can happen. So for contrib modules that implement plugins for other contrib modules, there will be chains of 2-3 issues to get it all done. Deferring removal of that to Drupal 13 gives a solid three years for all of that to filter through.
Procedural hooks as a whole I think we would definitely defer removal to Drupal 13 - it's going to affect almost every single contrib and custom module.
But specific hooks like hook_module_implements_alter() we'd probably still remove in Drupal 12 because they have tricky bc layers to deal with, especially if it makes other conversions and clean-up easier to do in Drupal 12.
@mglaman linked to https://github.com/phpstan/phpstan-deprecation-rules/issues/81 in slack, which seems like it could solve this at least on the phpstan/rector side. I don't know exactly how, but having the removed in version in a structured way ought to allow us to ignore it conditionally based on something. Test coverage would still show triggered deprecations, but ✨ Adopt #[Deprecated] attribute Active might enable something with that too.
With 📌 Defer disruptive 11.3 deprecations for removal until 13.0 Active , and also OOP hook + annotation deprecations all slated for Drupal 13, but likely to be introduced in Drupal 11, there will be quite a lot of deprecations that could potentially ignored by projects solely focused on Drupal 12 compatibility.
Moving to needs work for #50.
2. Administrative toolbar content navigation gets replaced by:
shortcuts navigation (one menu item)
content navigation (six menu items)
administration navigation (seven menu items)3. Administrative toolbar footer navigation:
help navigation (one menu item)
user navigation (one menu item)
It looks like this could probably be achieved by moving the aria markup to the individual navigation block plugins.
Committed/pushed to 11.x, thanks!
Moving 📌 Add php-tuf/composer-integration to core dependencies and governance — for experimental Automatic Updates & Project Browser modules Needs work from the alpha to beta sections.
Re-titling because I keep thinking this is about the governance issue and security review, which are more or less done at this point, whereas we still need to actually add the dependency to core. Would be good to get an MR up to add it.
I forgot this issue existed and opened 📌 Require password confirmation to install new code Active against project browser. Cross-linking the two issues.
This is looking promising, adding various review tags.
I will implement basically what I wrote in #15, but @catch was concerned in #16 that would still require a flat JSON structure.
AFAICT he thought that because it'd store both config and module dependencies:config: - a - b module: - x
But that's easily transformed to:
config:a config:b module:x
… at which point it's a flat list that is trivially queryable :)
hmm this wasn't actually what I was concerned about, my concern was about trying to store multiple values in one column. I did not think about imploding the array into a literal string, but pretty sure this also will have scalability issues. How it would compare to a JSON query I don't know.
The problem with the implode() is that MySQL LIKE queries can only use indexes from left to right until they hit a wildcard, so if you have an index on a string field, you can do a query like:
SELECT * FROM bar WHERE baz LIKE 'something%'
and that is fine.
But if you need to get rows matching an arbitrary part of the string, like config:b
in the example above the query is this:
SELECT * FROM bar WHERE baz LIKE '%config:b%'
And this does not use an index. Random blog post from google with more detail: https://planetscale.com/learn/courses/mysql-for-developers/indexes/index...
On final I think this is case where we'd prefer it never existed, let alone that someone tries to extend it or call it directly, so even more internal than controllers etc.
I applied the comment suggestion which I agree reads better.
Got turned around on the implementation question, turns out it was right in the first place, but in the process added a long comment to that.
Yes that also looks out of scope, might be better to revert those changes if the answer to the last two bullet points is 'yes' and open a follow-up. I think it's fine to do property promotion/remove constructor docs when already changing a constructor, but otherwise best handled in dedicated issues.
📌 Clarify/replace 'stage' terminology in Package Manager Active is RTBC.
I left a proposal and also question on 📌 Package manager/ Automatic Updates should disallow composer patches by default Active which could use review/help - the actual code changes in that issue should be very minimal.
This validation is done in ComposerPatchesValidator
, currently it checks if composer patches is a root composer dependency, and issues a validation error if it's a dependency anywhere else and not root.
Probably what we'd need here is a settings.php flag to enable that behaviour, defaulting to FALSE, and then only allow composer patches as a root requirement when it's TRUE. This would force people to make an explicit choice to use package_manager with composer patches, rather than potentially doing it by accident/without thinking about the consequences.
My main concern here is that someone installing Drupal CMS (and eventually site templates) via the UI, never gets into a situation where they have to manually edit JSON to update their site. Requiring a settings.php flag would mean that only pre-release versions of site templates could ship with patches applied, which is fine for testing, but they would need to be committed upstream before a stable release and the composer dependency removed in order to work with automatic updates/project browser.
I'm not sure how this interacts with 📌 Dependencies should be 'unpacked' to the root composer.json and merged/resolved Active - e.g. if recipes specify dependencies on composer patches, and those then get unpacked to the root composer.json, will that trigger the validation or not? From reading it, it looks like it prevents that situation from happening. I think if the answer to this is that it's impossible for that to happen and it will produce a validation error, we could move this to a stable rather than beta blocker.
Just revisited this issue based on 📌 Require password confirmation to install new code Active and I think we might want to split the UI into its own issue - if we use development mode to suppress password re-authentication, we either need to not have a UI, or put the development mode settings themselves behind password re-authentication.
Committed/pushed to 11.x, thanks!
Updated the issue summary.
Went through the MR twice, there are no logic changes, just the renames. All the renames make sense to me. In some places you can see the mis-match with the docs that will be updated in the follow-up, but even with the mismatch it's clearer because it's more obvious what's being referred to.
We won't completely remove the stage/stager concept because that's also in composer stager, but I think it's fine if we're 'staging sandboxes' using a Drupal adjacent third party library - composer stager has less concepts to balance than package manager / automatic updates / project browser.
If we find other small naming issues we can always address those during beta or future minor releases after stable with deprecations, but the large number of changes here shows why this needs to happen during alpha.
Automatic updates ships its own version of package_manager still, so should be able to handle any compatible issues smoothly by the release that removes the package_manager module also ensuring full compatibility with core package manager.
For sites that include both automatic updates + project browser, the transition from contrib package manager to core package manager will also be handled by automatic updates, but project browser itself will likely need versions for before/after 11.2 - that will have to happen for a Drupal CMS release, and the quicker we do all of this the better so we don't have to do it when things are even further along.
Moving this to RTBC which means I can't commit it, but if it's still in the RTBC queue without objections in a week or so, additional RTBCs would let me commit it in that case.
#5 looks really good to me, it's immediately obvious what the new terms are likely to refer to, even when I'm not sure what the old terms refer to - so that's an instant improvement in terms of both self-documenting naming as well as disambiguation.
Given the module is still alpha, and this is a blocker for beta, splitting this into code-naming + docs references seems fine to keep things reviewable. We can probably get both issues done quicker than trying to do it in one go.
When implementing this there's actually not really a race condition as described in #29.
If we implemented cache tag purging outside of drupal_flush_all_caches() then it would be possible for cache entries to exist with an 'incidentally matching' cache tag checksum. However, when we purge immediately after emptying the cache bins, there should be no cache entries created before the tags are purged, everything gets reset at the same time. There would have to be entries written literally during the purging itself for there to be a problem. This is about as likely as an entry being written in one cache bin that's just been emptied based on cached information in a table that's just about to be emptied - e.g. no worse than it is now.
So given that, what's in the MR might be enough.
Implemented #39.
🌱 [Meta] Move module.theme.css files to Classy or Seven Postponed: needs info is open from the child issues, and was rescoped to Claro.
It's not clear to me if the child issues represent all the intended work here or not. If they do, we've got one or two left and it could be closed. If it doesn't, then the entire issue summary needs a rewrite with clear next steps.
fwiw #40 sounds like how it should always have worked - there are plenty of ways for people to get to their own account.
That's a good additional test case...
The reason .js .js-hide doesn't work is because the .js class is added by... JavaScript so it completely undermines the point of the original MR. Obvious once you realise it, but wasn't when I tried it out to fix the views issue.
I could not think of another good way to add additional specificity, so went for !important
This works for both test cases, but let's be honest I haven't written much CSS either for money or for free since about 2009 so there might be a better way.
Explicitly tagging for manual testing just in case I messed up the steps to reproduce too.
Left a couple of (reasonably horrible) questions on the MR.
I can't remember exactly what we ended up doing for update module, but pretty sure nothing much changed since 2012 so we could probably still convert it to this API once it's available.
Looking at this again, it does look like the garbage collection only works for file entities because the only place where
Image styles as a whole can be flushed, which will clean anything remaining out, however also any new system would also have to add garbage collection both for styles and individual images.
But... I don't see how that can be achieved with the current proposal.
If a component defines a 2500px responsive image style which generates say five different image derivatives, then the hmac in the MR will ensure that no-one can generate a 2499px responsive image style arbitrarily. However, if a site stops using that component (swaps it out for one that's 1800px instead), how will the system know that it needs to delete all of those images (or not if the component is used elsewhere in a different layout)? This could be hundreds of thousands of derivative images on disk to worry about.
Equally, because the hmac only validates that the request is a genuine Drupal request and doesn't rely on configuration, even if there is some kind of event that deletes images when the last usage of a component is removed, nothing stops cached HTML requests from regenerating those images again because the URL is always going to be valid. You might not be able to regenerate lots of URLs for a DDOS attack but they might be referenced from internet archive or similar (or still in a CDN). Both image styles and asset aggregates have explicit protection against this kind of stale file disk filling situation.
Committed/pushed to 11.x, thanks!
For me personally, Creative Commons licenses are not software licenses at all. That means we should treat StackOverflow as documentation (or a discussion forum etc.), and if code follows that documentation and references the documentation it comes from, that is compatible with with a Creative Commons license and does not imply that using the code itself adopts that license.
Code on stack overflow is not released as code, it's embedded in a text document as an example, it's not even like a gist which might be a small standalone program.
Obviously I am not a lawyer, and also I have zero interest in whether this argument would stand up in court, and doubt it would ever need to.
So I think the main thing we should look at is testing existing functionality with 11.2 alpha/beta?
Yeah I think that's great. I think the canary jobs cover some of this but can't remember whether it tests 11.1.x-dev only or also 11.2.x-dev.
If there was a new minor branch for 11.x compatibility, then it would be possible to check things like new config deprecations etc. against that - it's possible a couple of recipes need updating, theoretically nothing will break, but less to do for 12.x if those are done.
For me personally I'd like to add new assertions to performance tests that are only available from 11.2, so if there was a minor branch for 11.2 compatibility, I could open an issue + MR to update those tests.
Point is, the source image could be very large or very small but the output dimensions are the same.
Doh, of course that makes sense, sorry it took a while.
So if the image style is for a small thumbnail, just enable the convert to webp plugin. If it's for a hero image then enable the convert with fallback plugin (or just convert to avif if you know its supported).
This makes me think we could add a global setting to configure this (with a default) if we want, and just go by the image style dimensiions - you're right that it's possible for someone to set this up correctly manually without any additional features, but there are already a lot of steps to setting up responsive images styles + formatters etc. so if we can do it automatically with the option to tweak, this wouldn't hurt.
If your image style is a 16x16 pixel thumbnail and your original image is 100MB the JPG thumbnail will still be very small.
Yes but we're talking about whether to convert to AVIF or webp, not whether to run the image style at all.
There is logic in DateFormatter::format() to dynamically build a max-age based on when the resultant date will turn over. So if there are 200 days until the end of the year, it should produce a max-age that will last for 199 days, but if there are two days, it'd last for 2. Original issue/discussion was in #2500525: Time ago/hence date/time formatting breaks caching; needs appropriate max age → .
However, I think for rendering dates (for comment posted timestamps etc.) rather than diffs, we concentrated on 🐛 Allow TimestampFormatter to show as a fully cacheable time difference with JS Fixed , this relies on js to transform the timestamp into the date, which won't work for tokens/metatags very well.
I think that if we could determine the maximum granularity of the date format, we could do a similar max-age trick for any formatted dates too - that would work really well for year, less well for minute. Don't think there is an issue for this.
More questions though: 🐛 [pp-3] Bubbling of elements' max-age to the page's headers and the page cache Postponed is sort of related but would more likely cause a bug in the other direction (page cache not respecting the max age), so do we actually know where the uncacheable cache metadata is being added? I'm not sure core by default will actually cause this behaviour so it might be a contrib module setting the max-age to 0.
Also agreed with @pameela in #8, is it needed at all?
Probably the simplest solution here would be:
1. Add a CacheTagsChecksumPurgableInterface with a ::purge() method and implement it in the database backend.
2. Call this method on all cache_tags_invalidator services that implement the interface, as late in drupal_flush_all_caches as possible, definitely needs to happen after plugin caches are cleared, not sure whether before or after router rebuild or not.
3. Open a follow-up for the potential race condition described in #29.
Ahh apologies from me too, I missed that you were already doing the rebase (although I think I ended up rebasing on top of your in-progress rebase).
These are one of few MRs I don't mind rebasing because it's nearly always because the numbers are going down :)
This isn't possible to test.
We should open a follow-up to look at whether the same trick could help in other situations, one I can think of might be cache garbage collection, which runs a delete against potentially a lot of rows.
Another problem here is that we need to know the extension when rendering the image style URL, not generating the image style, so it would need the image size at that point - possible to get this but might add some overhead to the URL generation itself.
I would have though the threshold would be based on the file size of the original image (e.g. what @andypost says in #6), so wondering why you think it would be useless?
So if the original image is 4mb - avif.
If the original image is 50kb -> webp.
No idea what a good default cut-off point would be though.
Dimensions - I would assume a 1200x1200 jpeg would still see benefit if it was converted to a 1200x1200 avif or webp though?
Agreed that the eventual size of the image is not feasible, no way to know that.
Also I just realised we also have the mime type of the original image available, so we could opt not to convert images that are already webp or avif - this feels obvious but I think the current convert plugins don't include an opt out for mime type either. I think there are rare but non-zero situations where image style conversion can increase file size, if an image was already well optimized manually.
Rebased again.
There's already a PoC in https://git.drupalcode.org/project/image_style_dynamic/-/tree/8.x-1.x?re... for this.
OK but from that page,
In order to protect sites from DDOS you can configure the allowed query strings inside
image_styles_dynamic.settings
this is not very encouraging.
But where would you choose the responsive image style in this scenario? This should not be up to the content creator to decide. The component should always render with the same (responsive) image style.
This seems like something for a site builder to configure. Even if not using responsive image styles as such, there needs to be settings for things like whether to crop to specific aspect ratios etc. as discussed above, which should never be exposed to content creators on a per-image basis either. So there has to be some kind of configuration step for the component not just within the component at the content creator level, and then it can happen there.
I somehow had the impression that the garbage collection of images was relying on file entities but after reviewing this again, it doesn't seem to be the case.
Yes it has nothing to do with file entities, this is why it's good to verify requirements before thousands of lines of code are written, even if an LLM is writing the code.
Also, there are settings that should not be in the component itself. For example, the image types that are generated should be a global configuration.
If the image type is a global setting, then why is the MR passing it to the component as a prop in the first place? And why is the component controlling which types are available? I think the answer to this is 'because the MR was written by Claude and posted without human review without clear instructions' but that's why I asked for an issue summary here defining the actual requirements.
Also if for some reason the format (or anything else) is taken from global configuration, then translated to a string to pass to the component, how is this translation layer different to what would be necessary to use a responsive image style to generate the relevant attributes and URL from to pass to the component?
It would be very hard to iteratively address this in the responsive image module. I think it would be better to build an alternative module in contrib,
Whether the work was done directly in core or in contrib, it would still be a coherent module in its own right, and not an entire subsystem hidden in experience builder.
Rebased.
Cross-posted with nod_, it's a CSS specificity issue - Claro's .button display: inline-block; is overriding the .js-hide.
Pushed a commit to https://git.drupalcode.org/project/drupal/-/merge_requests/11855#note_49... which seems to be enough.
Instead, I'd love to have a a tree of components generated first, then in a second step have it rendered.
This sounds like experience builder, and when experience builder is controlling the entire page, then it would probably apply to both the 'site chrome' and any entity content so effectively the entire page - although only when it's being used in this way.
it could have some explicit support for lazy-rendering/generating a sub-tree of components, but I think that should be explicit and used only rarely.
This is already explicit in the render API - you only get lazy rendering if you use #lazy_builder, and then placeholdering can be determined automatically but doesn't have to be. And you only get a render cache entry if you specify #cache keys and a #pre_render or lazy_builder callback.
However 'using it rarely' would go against a lot of current work in 🌱 [meta] Placeholder-driven performance improvements Active and 🌱 Adopt the Revolt event loop for async task orchestration Active as well as the existing render cache/dynamic page cache.
There are ways to provide the same async behaviour without actually using placeholders as long as you can iterate over items at the same level of the tree, for example see the implementation in 📌 Views entity rendering should use a lazy builder/placeholder Active which doesn't rely on #lazy_builder but does allow for async rendering. So we could reverse or partially reverse some of the placeholdering but keep the async, as long as the implementation allowed for a similar appropriate place to use revolt/an event loop.
But I don't see how to do render caching without #pre_render/#lazy_builder + #cache keys being used at least as much as it is now. When e.g. a views listing is not in the render cache, then you need to build up a full render array to render it, but when the views listing is cached, you only need the view/display ID - the whole point of render caching is to avoid having to run the listing query and load and render all the entities within it.
Since the entries in the cache only last for one request, it shouldn't really make a difference if the service is 'different' - e.g. it's not like if we swapped a persistent cache service around. So I can't think of too much that would go wrong in the issue summary except perhaps someone using autowire in their runtime code, but using the entity.memory_cache service name in a test. However even this would only cause a test failure and not a runtime issue.
In general, code that wants to use a memory cache should create its own service and inject it anyway, rather than chucking everything in the same place.
The one thing I'm wondering about though is whether we should also deprecate the new service - then we could tell people not to use it an define their own memory cache service instead. If this is controversial it'd be easy to split that to a follow-up for more discussion though.
Responding to the new issue summary (thanks for writing it up):
Image styles and responsive images are configured in config. This means that using the existing system requires introducing dependencies on the image style and responsive image config entities. This goes against the SDC principle where everything a component needs is supposed to be contained by the component.
The SDC principle is that it should receive the props, but the responsive image style config itself doesn't need to be a prop - instead the SDC component could receive the image URLs + dimensions + alt text, which can be built in advance based on the data stored in Experience Builder itself and passed in - just enough information to generate the img + srcset (or picture element) from.
Since the URL or URLs contain all the necessary information for the image style callback to render the image, the SDC doesn't need to know anything about anything else - it can render the markup/attributes based on what's passed in.
At the moment, any component library would have to re-implement all the various configuration options in the SDC added here in order to support responsive images, whereas really they should just be able to take the minimum set of information to build the markup.
Unless the only way to render a responsive image in XB is going to be nesting this component in a slot? Also what happens if a component wants to accept either a responsive image or an external one (which iirc is a requirement)?
The current implementation is shoe-horning half a dozen configuration options into the component, including whether AVIF or webp should be used. In general, avif or webp feels much more like a site-building task than a content editor one - why add the load of choosing this every time? There could eventually be even more formats available, as well as composite formats, like avif with webp fallback, potentially even with a file size check if we do 📌 Consider falling back to webp based on filesize Active . Is every combination of those going to have to be provided in the SDC as they're added? This really doesn't seem to be something to configure for every XB component.
On top of this, the existing image style system provides a lot more options than are mentioned here: cropping which is necessary when uploads can be different aspect ratios, focal point crop etc. I imagine Experience Builder would also want to support these - are they going to need to be baked into the SDC component too?
The responsive image module in Drupal Core is very flexible and from that perspective works well. However, it is very hard and tedious to use.This is a reason to improve the usability of responsive images in core, not to build an entirely parallel system that may or may not be able to do the same things and can only be used by experience builder.
I'm not sure what the answer to #3 is. Updating the issue summary to make this easier to find.
This looks right. Double checked that the other two methods are actually called by ::updateAll() already just in case we'd missed something there too, but that looks fine so this is just bringing the three together now.
Thanks! Moving this back to fixed.
Makes sense to just remove it, the error message is decent without it.
📌 Consider falling back to webp based on filesize Active has a possible approach to the trade-offs between AVIF and webp for filesize.
The post update / ViewsConfigUpdater method added here doesn't handle deprecations.
views_post_update_update_remember_role_empty
and views_post_update_views_data_argument_plugin_id
both do implement it so could be used for examples.
🐛 Add proper deprecation notices in config entity presave bc layers Active has more details. We can open a follow-up to implement that, or revert this and add it back with support, but one of those should happen before 11.2.0 so re-opening for now.
1/500 is enough to prove it fails, and we know from the other issue why and that using machine name will 'fix' it, let's switch to machine name.
I just now realised - what if we were to tag all these tests with @group legacy with a @todo to un-legacy them when mink 2.0 comes out, but we don't know when that would be, and we'd miss any other deprecations that are non-bogus in the meantime. We don't lose much by using machine name here so let's go ahead.
Committed/pushed to 11.x, thanks!
This desperately needs an issue summary update with the following:
1. What is the requirement for XB?
2. How far does core's existing image style and responsive image support go towards meeting #1. If there are gaps, what are the specific gaps and do they already have existing issues - and is it technical capabiities, usability etc.
Yes it should, looks like I lost track of the tab...
Committed/pushed to 11.x, thanks!
The test failures are (mostly? all?) due to known issues - added to issue summary.
Why is this re-implementing responsive images and image styles almost from scratch?
Annoyingly this is not achieving what I hoped it would achieve, which is increasing the number of multiple loaded path aliases in conjunction with 📌 Try to replace path alias preloading with lazy generation Active .
This is because the formatter builds a render array with a Url object, that hasn't yet been converted to a URL, so the Url::toString() call actually happens inside the foreach in field.html.twig which is not Fiber aware and won't be.
To make this work we'd need to render the field items before they get to field.html.twig or something like that.
However, the other way to get to the same eventual place is 📌 Support auto-placeholdering for blocks placed in Layout Builder Needs work - if e.g. a term reference field is referenced via a field block in layout builder, then we get the same desired effect as long as other placeholders on the page also render URLs. Similarly with 🐛 Placeholders/#lazy_builder is not supported for block component rendering Active .
Generally marking this postponed, not sure exactly on what yet.
Updated the issue summary.
This mostly helps when rendering tabs, but because we don't have admin performance tests, it's not picked up in test coverage yet.
I rebased the branch on top of 📌 Deprecate RendererInterface::render()'s sole $is_root_call parameter Needs work so that it's easier to test alongside other issues later.
Actually postponed on 📌 Deprecate RendererInterface::render()'s sole $is_root_call parameter Needs work .
Rebased on top of that to check something.
This makes sense to me and settings.php seems like the right (only) place. We could probably write to it at the same time as writing out to database settings so it's always there on new installs?
Backport looks good. Test failures are unrelated.