🇬🇧United Kingdom @catch

Is it worth discussing any specifics about arrays?

on arrays we have "The parameter type is not a single class, interface, or scalar type." which means all arrays would need @param docs still.

This is a policy document, not a governance document. Also, I think it's not accurate; the committers would ultimately make the decision, with input from subsystem maintainers and other contributors

This was changed from 'developers', I don't think we can word this so it's only about core - it includes contrib modules too.

nod_ → credited catch → .

@xjm on contrib, I think we could try to promote 🌱 Guidelines for semantic versioning and Drupal core support Active to an actual documentation page somewhere.

There is also 📌 Defer disruptive 11.3 deprecations for removal until 13.0 Active which means the list of APIs deprecated in 11.x and removed in 12.x is going to be quite short. And a lot of 11.x API additions were backported or otherwise made backwards compatible with 10.x (like OOP hooks with #[LegacyHook]).

We very much on purpose do not release or EOL any major or minor version on the December security window. :)

What does this mean if we do a security release on December 16th though? If we don't release it for 10.x, then it wasn't really supported a week earlier either.

Having said that, I would much prefer to close this issue with 'in December' and sort the question of a December 16th 2026 release out if and when we actually have to do one.

I spoke to the security team in slack and got a handful of +1s and no objections before this issue was posted, and also linked the issue once it was, so we are sorted there.

I don't think this needs addditional sign-off from Dries - it's just adding details to a schedule that was previously already outlined.

This does need a change record - contrib modules could be relying on this in test coverage for example, don't think we need a release note though. Went ahead and created a quick CR.

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

OK that all makes sense. Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

catch → created an issue.

Committed/pushed to 11.x, thanks!

This will need a backport MR for 11.2.x, although I also don't think it hurts if it's 11.3.x-only?

Went ahead and committed/pushed this to 11.x and 11.2.x so it's in 11.2.0-rc1, thanks!

Made one more change here directly on the MR - deprecation was for 11.3, but this is breaking things in 11.2 with to the fallback class loader - I'm not entirely sure if the fallback classloader is making things worse, or if it's dealing with a regression elsewhere but failing to fully compensate for it.

Last couple of commits look good.

Needs another rebase.

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

@mherchel - best to re-open issues if there's a problem or create a follow-up, it's easy for comments on fixed issues to go missing.

I've reverted this from 11.x and 11.2.x for more discussion. We could potentially add the library when text fields are rendered too.

I think #12 is fine, if we default to Drupal 13, then in cases where we need to deprecate for removal in Drupal 12 we still can. As long as it's not a change that will affect hundreds of modules then the overall effect is the same.

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

Sorry I think we should add the return types in their own issue, especially given the short amount of time before 11.2.0, back to needs work for that.

OK that makes sense. Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

There's no change to module discovery here, only the test classes themselves.

Something like this.

Just found 🐛 ExceptionDetectNeedsInstallSubscriber should not run on http exceptions Active too.

catch → created an issue.

Also this is wrong in general.

RouteProvider::preloadRoutes() is getting called twice - once with <current> and once with system.404, both load the route from cache because the RoutePreloader::onRequest() never gets called.~

One lookup is for the front page cache context, and the other is for redirect destination. So I was seeing something different to what is actually happening. There might be a saving here, but it's not an issue of the preload routes being loaded twice.

Nope cache tags properly checks isMainRequest() already.

Exactly the same thing happens with the cache tag preload subscriber, this might be small enough to do both in one issue.

OK I double checked what's going on.

The subrequest itself is cached in the dynamic page cache, e.g. this is what the cids look like in cache_dynamic_page_cache when you're logged out and browse to /abcdef

| response:[request_format]=html:[route]=system.40435786c7117b4e38d0f169239752ce71158266ae2f6e4aa230fbbb87bd699c0e3                                                                                                                                               |
| response:[cookies:big_pipe_nojs]=:[languages:language_interface]=en:[request_format]=html:[route]=system.40435786c7117b4e38d0f169239752ce71158266ae2f6e4aa230fbbb87bd699c0e3:[session.exists]=0:[theme]=olivero:[url7cOhVamWazkPEBofpmp46A6Nx9YVZ6IP27D-joYI6AY |

What doesn't get cached in dynamic_page_cache is request for the 404 page itself, e.g. /abcdef.

However, caching this would not really work in the context of dynamic_page_cache - it caches by route rather than per path, and by definition /abcdef can't be routed, if it could it wouldn't be a 404.

If we cached by path in this case, then there is no particular reason it couldn't be cached, but the internal page cache module (page_cache) already successfully caches the 404 response by path. This means the dynamic_page_cache would have a very low cache hit rate. Similarly, because the response sends cacheable http headers, varnish or a CDN can cache the HTML at the edge too. Once it's in either the internal page cache or an edge cache, then it won't hit dynamic_page_cache anyway.

Did some quick profiling of what it looks like with xhprof, and found 📌 Avoid preloading routes in subrequests Active which would be quick win in this situation though. There might be other things we can optimize away too.

catch → created an issue.

📌 Add an API for comparing the (current) route name that takes into account deprecated routes Active is likely to rely on the route preloading even more than we already do.

Not strictly related to trying to factor it out, but something I just noticed:

On a 404, we make subrequest to the configured 404 pages, and we preload routes twice, this probably needs a quick spin-off issue with an ::isMainRequest() check.

X-Drupal-Cache-Max-Age: 0 (Uncacheable)

This doesn't apply to the dynamic page cache, only the internal page cache. There is a separate header for that:

X-Drupal-Dynamic-Cache

It does look like that's not cacheable with the standard profile though.

📌 Block status code visibility condition should use a status code cache context Active is related.

Can you check whether the same problem exists with 11.1?

@mondrake that all sounds good. I think defaulting to phpunit 11 is fine

Yep let's tag it for 11.3.0 release highlights.

Yes let's revert for now - hopefully we have enough to go on from the failed runs to figure out what's missing here.

@quietone noticed that the tickets mentioned in https://gitlab.com/rugged/rugged/-/issues/?label_name%5B%5D=OSTIF-2023 are still open, I don't think we can close this issue until those issues are actually closed - whether because they're fixed, or because they're closed as 'won't fix'.

Gave this a quick manual test locally and it worked for me.

I think we should create a follow-up to add phpunit coverage for this - might be possible to add a couple of lines to the existing coverage even.

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

Should this be linking to https://www.drupal.org/docs/develop/core-modules-and-themes/core-modules... → instead - not that there is much there, but it seems like the right place to link to if there was more.

Symfony 7.3.0 just got released.

@jonathan1055 no I'm not sure it's a bug in gitlab templates at all. It might be we have to revert 📌 Deprecate TestDiscovery test file scanning, use PHPUnit API instead Active or commit a follow-up to 11.2 to make things work here. However contrib not being able to test against 11.2 is definitely a critical bug in general.

I do think it's worth a separate issue to stop testing against core-recommended so that things like upgrading phpunit conditionally are possible.

I added a note to the 9.5.9 release notes mentioning it may require a php-fpm/apache restart. Don't think there is anything else we can do here. https://www.drupal.org/project/drupal/releases/9.5.9 →

Laminas is no longer a dependency of Drupal core, partly due to their release cycle being incompatible with ours, which led to the fork here in the first place.

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

I think we should do this.

A language selector would allow for a closer equivalent to the current behaviour for people that need it, but the page just does not properly work as it currently is, so I think we could open a separate issue to add that, and keep things simple here.

This needs git archaeology before it can be committed - we need to know whether this was never used, whether it should be used, whether it recently became unused because of other changes etc.

The dateString argument has been sent since:

commit b4435decef6ebbfa2c5e9da23c2916fca3b04f57
Author: Angie Byron <webchick@24967.no-reply.drupal.org>
Date:   Fri Nov 21 04:33:28 2008 +0000

    #11077 follow-up: adding missing timezone.js file.

The original menu callback was this:

 
 /**
+ * Menu callback; Retrieve a JSON object containing a suggested time zone name.
+ */
+function system_timezone($abbreviation = '', $offset = -1, $is_daylight_saving_time = NULL) {
+  // An abbreviation of "0" passed in the callback arguments should be
+  // interpreted as the empty string.
+  $abbreviation = $abbreviation ? $abbreviation : '';
+  $timezone = timezone_name_from_abbr($abbreviation, intval($offset), $is_daylight_saving_time);
+  drupal_json($timezone);
+}
+

So this was added in 2008 with the original issue, and has never been used.

But also - I think we should open a follow-up to look into removing the route and AJAX request, if it's only a fallback for non-compliant browsers, then we should be able to fall back to the user selecting their own timezone.

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

That is so much more straightforward. Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

One comment on the MR.

Yeah I think this is a duplicate of 🌱 [policy, no patch] Document named arguments BC policy Fixed . Going to close as outdated.

One question on the MR - to me it looks like the entire tracking topic should be removed.

quietone → credited catch → .

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

quietone → credited catch → .

Looks good. Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

quietone → credited catch → .

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

The core-recommended requirement is because core requires sebastian/diff - but any version 4-7, but then phpunit 10 requires 5.1.1, so co-recommended is getting the version from there. This feels like a fundamental issue in core-recommended - it's checking composer.lock, but composer.lock is based on the full set of dependencies including dev.

Only thing I can think of is gitlab templates switching away from core-recommended entirely, that should allow everything to resolve then.

Changing this to 'critical bug' because it's preventing contrib testing on 11.2.x

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

This is outdated after 📌 Correctly handle cache data instead of throwing an Exception in EarlyRenderingControllerWrapperSubscriber() Fixed - no more early rendering exception which is what this issue was trying to prevent.

In 📌 Make POST requests render cacheable Fixed we added render caching on POST requests, but only for cache hit, nothing gets written. That means we could still make this change safely, but it needs a comment update.

That issue is in. I think this is done.

Adding the release notes tag - we'll want to document the UI changes, and maybe the API changes if we think they could affect contrib, in the 11.2.0 release notes.

Also wonder whether we could do something about ajax-progress.module.css in a similar way.

e.g. could we move that to a separate library, and then have the AJAX system load it via loadcss so that it's non-blocking? Seems like more or less the definition of CSS styles that shouldn't need to be blocking. It's not impossible that a page immediately triggers and AJAX request but it's pretty uncommon.

The change record here is for the previous solution, which is also still in the summary. Does that need a follow-up issue?

Also, I think this could use a change record/release note to inform site owners they will get a config diff the next time they export config and why.

Couple of comments on the MR.

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

@tonypaulbarker

All of these image styles are opinionated and based on Olivero container sizes so the answer is that the art direction images will work as well as the others - with mixed results.

That makes me think it would be worth trying less opinionated image styles that rely on sizes - so that they work better across different themes, even if they're less customized for drupal_cms_olivero.

But what if we moved the breakpoints into that, and made drupal_cms_image depend on it...and then, rather than completely drop Drupal CMS Olivero when we have XB, we instead just made a 2.x branch of it that only contains the breakpoints?

I don't see how this would help because the new themes based on Experience Builder won't have the same breakpoints as Olivero.

Note that there is ✨ Add automated image optimization to image component Active , which I have no idea whether it's an XB blocker and it doesn't have a working MR yet, but the current vague state of that MR would only use sizes and not picture. e.g. #3515646-22: Add automated image optimization → but it's not clear whether that's by design or an oversight yet.

The idea here was to ensure that new core assets, like logos, icons etc. are optimized. So it is something like a step to ensure that's not forgotten. I don't think we can specify specific tooling since things change all the time.

@wim leers #3 is proposing an out-of-band update to a separate storage (column on the field table) with a counter though (e.g. a mass update triggered by config changes). It would require direct database queries against the field storage to implement that, they can happen in a transaction but they won't really be atomic IMO - e.g. it's possible for an entity to be loaded before the query runs, and saved after it runs.

So: AFAICT you were just talking about how many actual component instance slots exist in total in a content template ("distinct", 20) vs which ones are exposed (12).

Yes or more specifically, I couldn't think of a use case for consecutive exposed slots, so am assuming there'd always be at least one 'fixed' slot in-between. But the main point is that 12 would be at the top end, not an average.

Yeah that makes sense, if we have less commands, it's less js. Moving to [PP-1] on at least some of 🌱 Gradually replace Drupal's AJAX system with HTMX Active .

@catch mostly that would work but we do have some art direction

How much is it possible for the art direction to actually work across arbitrary different themes though? This isn't a rhetorical question, I don't know how much it work work vs. not.

Those two files comprise a tiny, minimalistic, yet complete, theme called drupal_cms_image. It could maybe even be hidden (assuming the themes page supports that), so that it can't be inadvertently uninstalled.

Could this be an actual contrib module that themes can depend on?

catch → created an issue.

I've updated the summary to clarify that, I hope it helps

Thanks, yes that does help :) it wasn't clear whether it was 'explicitly deferred' or 'stretch goal', but can stop worrying about it now.

Team Member: Name, role, bio, social links, photo
With listing page?
Demo content: 3 fake team members

This could theoretically be re-usable for other site templates like something NGO-ish or different kinds of brochure websites.

The change record should explicitly reference the config key that's being removed. There will be some cases like distributions with a full set of exported config that could end up with stale config after this change and will need to remove the key manually.

I'm confused by the update number here - left a comment on the MR.

Current security team policy on this is here:
https://www.drupal.org/drupal-security-team/security-team-procedures/dis... →

I think that eventually changing the policy would still overall be a security team decision (or some kind of joint core committer/security team decision), but it's impossible (or at least useless) to treat username enumeration as an issue to be handled in public until all known public issues are fixed, hence the change of project in #39.

I'd expect this or that client will not want people to revisit until fully graded (let's say it's an exam with some open questions - editing answers after finishing and before everything is graded would defeat the purpose).

Oh hmm I had not thought of that. So allow users to go back and see what they did after things are graded, but not to do it until then.

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

Blog Post
Title, body, categories/tags, author, published date -- we can pretty much reuse drupal_cms_blog for this

Does this mean actually re-use drupal_cms_blog or copy it? My initial reaction is that it would be much better if it was added as a dependency rather than duplicated, so that that different site templates and recipes in general, have a consistent blog content type (unless there's a very specific reason to diverge). Any changes would only need to be made in one place, only one content type to theme etc. It's much easier to split things later if there's a good reason to than it is to consolidate them back into one thing if they've already proliferated.

Behavior tracking

This has GDPR and general privacy implications (and front end performance implications and etc.) and don't think it should be in scope. There is plenty to get right here without adding a load of compliance work on top of it. Eventually stuff like this can be pulled in from project browser or similar.

There is no mention of XB here, is the idea to try to build this on top of XB from the beginning, or to add that in later?

Couple of questions on the MR - it's not really clear to me why 'revisit mode' doesn't already handle this - e.g. if you can revisit after a course is graded, I think you would be able to revisit before? But last time I tested that, it looked like that wasn't possible. I think if we changed that, then we might not need the setting at all?

If revisit mode is off, then stopping people from going back in until grading is complete seems appropriate still, although agreed that if we need this level of granularity, then a global setting probably makes sense here.

I haven't used this module.

Given that 404 pages can (usually) be cached in the dynamic_page_cache as a single entry, and should be very quick to serve, has anyone checked recently that it actually makes things any faster, rather than just adding slow queries?

@effulgentsia

Suppose in the future we want to be able to support Views (or even just EntityQuer(ies)) for finding all instance of a Heading component whose element prop is equal to h2 (or likewise get lists of component instances matching whatever other condition).

As well as @lauriii's answer, a single field would only make this easier at a per-entity-type or per-bundle level. As soon as you have differently named fields on different bundles, or even the 'same' named field on different entity types, then a single entity or views query on one field won't cut it. So from an auditing point of view, it doesn't make things any more complicated than they already are.

Also, wouldn't Paragraphs have a similar limitation to this, when you use multiple entity reference fields (i.e. multiple slots)?

I don't know how common this is, but yes. And similarly the use case where sites use layout builder for the top part of the content and paragraphs for the bottom.

package_manager has the same name as a contrib module, but because it came from the automatic_updates project, we didn't have to deal with identical project names, and that's the biggest complexity here.

e.g. automatic updates can create a new contrib branch without package_manager and both can exist at the same time on a site, and then the package manager contrib module will 'disappear' from the code base and the core one will take over.

But when the project name is the same, need to make it so that the contrib project disappears from packagist and that core provides it instead. I think this still needs manual intervention on Drupal.org but can't remember exactly what needs to happen.

#2980922: Applicable bundle field definitions defined in code should not have to manually implement hook_entity_field_storage_info. → and various other issues are open to improve support for bundle-specific fields.

I don't know if there's an issue to allow defining fields on bundle classes, but that would need to be postponed on those other issues.

An orphaned_reasons column would have similar problems to file_usage in core (which we still haven't resolved. We had to disable automated file deletion because in-use files kept getting deleted and the only solution we have is completely replacing the entire subsystem with something that tracks actual references like entity_usage).

If there's any kind of race condition, failed query, or similar error which can cause the increment/decrement to get out of sync, then either garbage collection will happen when it shouldn't, or it will never happen.

I think there are also information disclosure issues here too - e.g. how do we know that all consumers of the field, JSON:API, custom code etc will respect that only specific deltas are not available? This is much easier to do for an actual field where there is deletion, field access etc. to rely on.

@catch in #44: no, #3524406: [later phase] [Needs design] [PP-1] Provide API for finding and UI for surfacing dangling/dead component subtrees — aka garbage clean-up will be necessary even if we do that. Check it out — there's other ways to end up with dangling subtrees 😬

hmm, that issue seems like it's dealing with a data-integrity bug as a result of code going AWOL (similar to Views invalid plugin handling). For me that is very different to being forced to deal with a data integrity issue because of the way that fully valid and allowed configuration changes are reflected in the schema.

But I don't understand the other. We'd have to purge all component instances within an exposed slot that was deleted. So it'd be basically:

UPDATE {field_table} SET deleted=1 WHERE parent_uuid IS NULL AND slot LIKE 'my_deleted_exposed_slot'; for the content entity type+bundle whose ContentTemplate had an exposed slot deleted
the same for the {field_revision_table} revision table
… then gradually purging just those rows, not all fields. The challenge there is AFAICT the need to call \Drupal\file\Plugin\Field\FieldType\FileFieldItemList::deleteRevision() and friends.

This is about as far as I got trying to think what purging only components would look like, or well, it's a lot further than I got.

How do you go from "12 exposed slots" to "20 distinct slots"?

Hmm maybe I am missing something.

My understanding of ✨ Content templates, part 2: store a component tree in the template, and allow individual entities to fill in specific slots in that tree Active was that it allows for the following use case:

1. Parts of the template where the content author can add different components of their choosing.
2. Parts of the template which are 'fixed' and will be filled in via field content, or hard-coded to a specific block or something.

and that there could be multiple of #1 and #2.

If that is correct, then I would assume that when you have #2, you also have #1, hence going from 12 to 20, or at least, that you would have 12 exposed slots and 0 non-exposed slots. But do I have the wrong end of the stick?

But either way, the point is that I can't see people adding more than 12 of them, it seems like the sort of thing you'd add 1 or 2 of at most? And if so, then field-per-slot is a couple of fields, not dozens.

The static compression logic is very closely tied to aggregation. However ✨ [PP-1] Allow setting aggregation groups for js files in library definitions Postponed would allow libraries to specify that only files from that library should be aggregated together, and that would achieve more or less the same thing I think.

If files aren't aggregated and aren't in a writable directory then there'd be no way to write compressed versions to that directory.

Going to postpone on that issue as the most likely way to implement this.

The only known issue from that change is 🐛 PhpUnitApiGetTestClassesTest and PhpUnitApiFindAllClassFilesTest need to execute PHPUnit discovery before TestDiscovery Active , might be worth posting the error there for now.

Yeah the gain would mostly be in a single web head + cli situation. So mostly local environments and smaller hosting setups. As soon as you get to multiple web heads it's going to be marginal.

The only other option with ckeditor5 would be to move the module to contrib, so that it can release new versions when new ckeditor5 releases come out, and potentially be a bit closer to semver (new major for ckeditor5 majors, new minors for minors, new patch releases for patch releases). However that would be a huge change that would require its own issue and discussion.

I can't find a rector rule to break constructor params onto newlines though.

Maybe phpcbf has one? Would make it a two step process but might be worth it considering the actual commit would hopefully be one-hit.

While the log length limit doesn't help, 7m30s seems in-line with previous runs that had @group #slow and also faster than HEAD - so that might be encouraging?

I'm not sure how gitlab templates deprecation support works, is it possible to turn that off so the logs are quieter?