Account created on 13 October 2005, over 19 years ago
#

Merge Requests

More

Recent comments

🇬🇧United Kingdom catch

On webp I don't think it needs to be configurable for editors, but we just added AVIF conversion with WEBP fallback Active to core which uses avif if you have it (server side, all browsers that Drupal supports support it already), webp if you don't.

🇬🇧United Kingdom catch

This is probably a duplicate of 📌 Add a grace period to ChainedFastBackend when last_write_timestamp is ahead Active at this point - the other issue is much newer and the approach is different, but it's trying to solve the same problem.

🇬🇧United Kingdom catch

When I'm building Drupal sites, I composer require the dependencies I need then install them. Sometimes a dependency isn't needed any more, so I need to uninstall it, and then later composer remove it (because you have to deploy the uninstall before you can remove the code), this doesn't happen that often but it does occasionally.

Modules and their packages on composer don't necessarily match, so it's a fairly laborious process to audit a site for unused composer dependencies overall as opposed to requiring and removing as you go along.

When you update to a new major version of core, it's quite likely that some modules won't be compatible with that new Drupal version. Even if all the modules that Drupal CMS ships with are compatible with Drupal 12 or 13, this might not be the case for modules it shipped with in February 2025. In these cases, if a module isn't installed, composer remove is the easiest way to make your site compatible - but you have to know that it's not used and how to remove it.

This is going to be even worse for someone that's not familiar with composer on the command line or the nuances of composer requirements vs. installed modules, and it's exacerbated if sites start out with a large number of unused dependencies that were never installed in the first place.

🇬🇧United Kingdom catch

site owners are free to prune unnecessary dependencies as desired.

How do site owners prune those dependencies?

🇬🇧United Kingdom catch

I'm hoping to get rid of path alias preloading in 📌 Try to replace path alias preloading with lazy generation Active so that might not be a concern, once that issue lands.

🇬🇧United Kingdom catch

The metadata that impacts the version hash (or put differentlY: when the hash changes) could be updated (current implementation stems from #3528362: Deterministic Component version hash should depend not only on source-specific settings, but also slots + explicit input schema) could be updated to include the image restrictions imposed by the component developer.

If the rendered image size is changed by an SDC - say it changes from image and description side by side to top and bottom and then the image becomes full width instead of half width, then I would assume that's a purely presentational change that should be immediately reflected in all cases that the component is rendered? Otherwise you could potentially have mismatching versions shown even on the same page depending on when they were last saved?

🇬🇧United Kingdom catch

The biggest blocker is the disconnect between the compiled container before or after a module install/uninstall when that happens as part of a config deployment or update.

🇬🇧United Kingdom catch

The MR is back to green, but we need to figure out scope here. There are other sha256 and other hash usages in core we can probably convert.

Also not sure whether we want to spin-off PermissionsHashGenerator and any other logic changes to their own issues. Also noticed in the twig change that we're using Crypt::base64Encode() purely to base64 encode a hash but I think that's already the case with xxh3 so we could probably use xxh3 for that case too.

🇬🇧United Kingdom catch

Yeah makes sense to backport this one to 10.5.x too. Cherry-picked to both 10.x branches.

🇬🇧United Kingdom catch

I think this is fine to backport to an 11.2.x patch release. Moving back to RTBC for the cherry-pick.

🇬🇧United Kingdom catch

Can't see any reason not to use xxh3, probably got thrown off by it being 'new' but it was included in PHP 8.1

🇬🇧United Kingdom catch

Rebased to get a fresh test failure baseline. Removing needs tests tag because these were added.

🇬🇧United Kingdom catch

Combined MR with 📌 Pass RenderContext around in the Renderer Active is green now and removes about 500 lines of code.

To see a larger reduction in the number of database queries on cold caches, we need 📌 Use a lazy builder/placeholder for the top bar Active and 📌 Views entity rendering should use a lazy builder/placeholder Active - but those can happen independently of this issue.

🇬🇧United Kingdom catch

Opened 📌 Refactor _ckeditor5_get_langcode_mapping() Active for the ckeditor5 issue.

Revived 📌 Implement xxHash for non-cryptographic use-cases Needs work with a new MR and added the permissions hash generator change there - that issue might need splitting but at least it documents the thought.

OpenTelemetryNodePagePerformanceTest

I think this is a timing issue with the test - it looks like the image style request didn't finish before the performance data started recording. Added a couple of sleep(1) calls.

CKEditor5MarkupTest.php

Re-ran this test and it looks random.

🇬🇧United Kingdom catch

The fork was too far behind (early 10.x branch) to be able to rebase on, so cherry-picked the one commit that still applied to 11.x, recreated one more of @neclimdul's and pushed a new branch.

Also adding PermissionsHashGenerator here after discovering it makes debugging test cache changes unhelpfully difficult. We probably need to split that to its own issue though because it'll likely need constructor deprecations and a double check that it's definitely not security sensitive (I don't think it is though, it's a hash of permissions, not account information otherwise).

🇬🇧United Kingdom catch

ckeditor5 was a red herring, although that's definitely a small optimisation we can make in another issue.

The extra cache get is from this in Drupal\Core\Theme\Registry

    // If called from inside a Fiber, suspend it, this may allow another code
    // path to begin an asynchronous operation before we do the CPU-intensive
    // task of building the theme registry.
    if (\Fiber::getCurrent() !== NULL) {
      \Fiber::suspend();
      // When the Fiber is resumed, check the cache again since it may have been
      // built in the meantime, either in this process or via a different
      // request altogether.
      if ($cached = $this->cacheGet()) {
        return $cached;
      }
    }

Because the execution is now inside a fiber, this suspend happens, then when it's resumed the cache is requested again.

This was added in 📌 Add PHP Fibers support to BigPipe RTBC .

I think there's a question of how useful this is without 📌 Use revolt to prewarm caches during lock waits Active or async database queries etc., but this explains the extra cache get. For me that means there's nothing to do in this issue although we might want to revisit that elsewhere.

To get a useful diff, I had to remove the hash salt and private key from the user permissions hash - I think we might also want to open a spin-off for that because not only are the permissions hashes never really exposed anywhere, but also there's no useful information in a hash as such anyway - this would allow us to compare cache operations much easier when they're part of cache contexts.

🇬🇧United Kingdom catch

#18 might not be right, debugging I can't see an extra cache get - just that there's too many. So possibly that call just moved around. Will need to look more.

🇬🇧United Kingdom catch

It should. Also updating the issue title for what's actually happening.

🇬🇧United Kingdom catch

Not sure why there's an extra cache get in the performance test yet, but did a file_put_contents() of the performance_data object then diffed it, and this is the extra one (would be nice if it looked this nice in the diff output, there's also a load of hashes in cids that are different for probably no good reason):

                             [21] => ckeditor5.langcodes
                             [22] => ckeditor5.langcodes
                             [23] => ckeditor5.langcodes
                             [24] => ckeditor5.langcodes
+                            [25] => ckeditor5.langcodes

That comes from _ckeditor5_get_langcode_mapping(). Will open a spin-off to move that to a service and keep the mappings in a property etc.

🇬🇧United Kingdom catch

Pipeline weirdness might be fixed by giving yourself push access to the branch, I think I've had the same thing happen.

🇬🇧United Kingdom catch

catch changed the visibility of the branch 3496369-with-more-placeholdering to hidden.

🇬🇧United Kingdom catch

Looks like the idea behind making them private was because they might be temporary and be removed later, I think the MR here is fine for a quick fix, we can re-evaluate the constants themselves later.

🇬🇧United Kingdom catch

@vetchneons is this a regression due to 📌 Always rename dot files like Drupal 7 Needs work (which was just committed yesterday) or does it pre-exist that issue?

🇬🇧United Kingdom catch

@martygraphie can you confirm whether you're using linkit module?

@bsanders44 can you confirm whether you're using linkit module?

🇬🇧United Kingdom catch

This is probably 🐛 Future compatibility fix for v45 of CKEditor5 Active . Moving to needs more info until that's confirmed either way.

🇬🇧United Kingdom catch

Was able to come up with a unit test that simulates the assertion failure in ::executeInRenderContext() during big pipe rendering when 📌 Try to replace path alias preloading with lazy generation Active is applied, which was otherwise only visible in a couple of functional tests. Coming up with a minimal render array was not trivial - it needs both nested calls to ::executeInRenderContext() and also multiple Fiber::suspends() to trigger the assertion.

🇬🇧United Kingdom catch

Yes @zigazou if you find out which module was calling this function, that would be useful information to add here - can either get a backtrace or grep your local code base.

Either way the bug itself is obvious if not how it was triggered. Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

🇬🇧United Kingdom catch

Thanks for double checking.

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

🇬🇧United Kingdom catch

Closing this as a duplicate of 📌 [meta] Replace lazy service proxy generation with service closures Active which will change all of core's proxy classes to use service closures.

🇬🇧United Kingdom catch

I think we can assume that 🌱 Gradually replace Drupal's AJAX system with HTMX Active has a higher chance of being completed before either this issue or jQuery 5, postponing on that.

🇬🇧United Kingdom catch

Updated the issue summary now that PHP has officially changed to a 4 year security support policy. That means that if we adopt this policy, we get (maximum) 3.5 years of overlap with the minimum PHP version for each Drupal major release. It's not as long as we hope to maintain those releases for (e.g. Drupal 10 will be supported until December 2026 while PHP 8.1 is EOL December 2025), but I think it's a good balance between giving hosts (and our dependencies) six months to add support for the new PHP version vs. having a minimum PHP version that's already about to go out of support within a couple of years.

There's a new post at https://stitcher.io/blog/php-version-stats-june-2025 which shows PHP 8.4 at 14% of installs and PHP 8.3 at 34%, this means just under half the reported installs in that data are on versions that support Drupal 11, released 9 months ago. It does show PHP 8.4 adoption is a bit slower than PHP 8.3's was though.

🇬🇧United Kingdom catch

PHP's support cycle changed since we last discussed this, and there's now four years of security support for each release.

https://www.php.net/supported-versions

This means Drupal 10's minimum of PHP 8.1 is supported until December 2025, while we're intending to support Drupal 10 until December 2026.

Drupal 11's minimum of PHP 8.3 is supported until December 2027 etc.

I think the main consideration here is which PHP version we start a major release on, e.g. 🌱 [11.x] [policy] Require PHP 8.3 for Drupal 11 RTBC , so I'm going to postpone this issue on 🌱 [policy] Default to requiring the latest stable PHP release available when a new major version reaches beta Active .

🇬🇧United Kingdom catch

Pretty sure this hasn't changed in the meantime and is still worth looking into.

🇬🇧United Kingdom catch

More than once I wondered why this wasn't provided by REST module so that makes sense to me.

🇬🇧United Kingdom catch

Untagging for security review because this is a test-only change and clarifying that in the title.

If we want to remove --audit everywhere that could possibly use security review but given the --audit does literally nothing when run via package_manager at the moment, I don't see what it's doing that would be remotely useful.

I've opened Consider showing composer audit results in a report Active as a spin-off though.

Assuming that an audit call is added as part of a transaction that prevents a vulnerable package from being installed

Feel free to open an issue about this, but simply checking if composer audit returns any results isn't sufficient - it would for example prevent updating to a security release of a dependency, just because another dependency had a CVE issued in the last ten minutes or similar. The composer audit results would have to be filtered down to only whether the project is being newly installed, and composer doesn't actually provide a useful list of what is being newly installed. Create a Composer plugin that records what packages are being updated, installed, or removed Active is open to try to workaround that, but it would at minimum need to be postponed on that issue.

🇬🇧United Kingdom catch

Coding standards still require @param everywhere, it will change soon when 📌 Allow omitting doxygen when type and variable name is enough Active is adopted although only in specific circumstances.

This looks good to me. Committed/pushed to 11.x, thanks!

🇬🇧United Kingdom catch

Yep re-RTBCs of issues after re-rolls are fine, although it's also good to add a comment to explain that's what's happening.

Committed/pushed to 11.x, thanks!

🇬🇧United Kingdom catch

Also not entirely sure we need to split test module hooks, but can't really find a strong argument in either direction. Can always move things around later.

Committed/pushed to 11.x, thanks!

🇬🇧United Kingdom catch

Adding a c& p of the output to the issue summary. Out of time for the day but this looks great!

🇬🇧United Kingdom catch

Committed/pushed to 11.x, thanks!

🇬🇧United Kingdom catch

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

🇬🇧United Kingdom catch

Also I'm not sure #11 is really describing what the --audit flag does - it doesn't audit your composer project as such, it checks for security advisories. If there's a problem with a require or update, you get an error long before --audit would happen because it's (iirc) the very last thing to run.

🇬🇧United Kingdom catch

I can understand not displaying, however silently allowing a 'known vulnerable' package to be installed?

Currently any potential output from composer --audit is discarded by package_manager, so "silently allowing a 'known vulnerable' package to be installed" is the status quo.

The --audit flag is enabled for every package manager operation - this includes unattended automatic updates which could be run via a cli cron job. Possibly the output could be logged there, but logs are ephemeral (especially on smaller sites) so it could easily be missed even if it was logged. Also don't think the output is useful to show people if they're e.g. updating a specific contrib module and the CVE is for a completely different dependency of a different module.

So if we want to alert site admins to known security vulnerabilities in non-Drupal dependencies, then I don't think running with --audit and then throwing the results away is useful at all, it would be better to try to incorporate an explicit composer audit call into update status somewhere (e.g. only when package_manager is enabled) - then it would be available in admin/reports, integrated with e-mail notifications etc. It's pretty unusual to newly install or update a package that has a known CVE, much more likely a package that is already installed has a CVE issued for it and needs update.

Might as well tag for security review as well, but I think a new issue to add an explicit composer audit and show the results would do a lot more here.

🇬🇧United Kingdom catch

I think even if this was a bit disruptive for some contrib modules, it's covered under this in the bc policy:

https://www.drupal.org/about/core/policies/core-change-policies/bc-polic...

Test traits and abstract base classes should generally use deprecation where possible rather than breaking backwards compatibility, but they are still considered an internal API and may change if necessary.

Not having to add a baseline entry every time you use one of these traits is a very good improvement for both core and contrib which for me outweighs the possibility of someone potentially having overridden a trait method that's used from a base class they inherit from. The fix would be to add the equivalent type hint in the test, which would be fine with previous core releases too.

🇬🇧United Kingdom catch

I think this is safe to try after 📌 Bump Core to 11.2.0 Active

🇬🇧United Kingdom catch

📌 Get coffee data only when the search box is opened Active should have changed things so that the request isn't made until the search box is opened. If that's running all the time, something else is broken. Do you have e.g. gin_toolbar enabled?

🇬🇧United Kingdom catch

There's also 📌 Remove unnecessarily complex logic from tableresponsive.js Needs work .

This is very straightforward. We can try to refactor things in other issues, but stopping the page from completely locking up great.

🇬🇧United Kingdom catch

Just reproduced this on Drupal CMS - page freezes for about three seconds and chrome performance profiling pinned it down to tableresponsive.js, then searched and reached there again.

🇬🇧United Kingdom catch

Looks good and the CR lines up.

Updated the proposed resolution which was still out of date.

🇬🇧United Kingdom catch

Updated the previous CR to include this issue.

This isn't disruptive so can still be removed in 12.x, updated the deprecation version to 11.3

Committed/pushed to 11.x, thanks!

🇬🇧United Kingdom catch

Committed/pushed to 11.x, thanks!

🇬🇧United Kingdom catch

https://www.drupal.org/project/node_revision_delete has a helpful list of similar modules, and it's a very long list. Haven't found a clear winner yet.

🇬🇧United Kingdom catch

@yonailo can you confirm you're already on version 2.0.1 of coffee?

🇬🇧United Kingdom catch

Pushed a commit changing this to 8 instead of 4 per the discussion in #23-#28. Short version is scheduler and several other modules will have enough slower tests that 4 isn't enough space to start them at the same time. 8 is more likely, and still considerably lower than 32.

🇬🇧United Kingdom catch

Is 📌 Introduce PHPUNIT_FAIL_ON_PHPUNIT_DEPRECATION Active enough to trim that back down?

🇬🇧United Kingdom catch

The library etc. hooks are more like 'html rendering' hooks than theme hooks but the hook documentation organisation predates us having a render/library/asset layer distinct from the theme system or form API.

We might want to revisit this at some point but don't feel strongly enough to block commit on moving them back or elsewhere.

Committed/pushed to 11.x, thanks!

🇬🇧United Kingdom catch

I asked about the deprecation of the preprocess function itself in slack and @berdir explained that (unlike most procedural hooks) there is a reasonable amount of contrib calls to various preprocess functions, usually from other preprocess functions.

Couldn't see anything else wrong here and agreed on not trying to add a helper for the sorting.

Don't think we need a CR for the deprecations because this is all internal code anyway. Updated the deprecated in version from 11.2.0 to 11.3.0 on commit.

Committed/pushed to 11.x, thanks!

🇬🇧United Kingdom catch

(is this for entering new credentials that would be used in LMS xAPI to connect to this LRS

Yes.

But no LMS activity (e.g., H5P completions) results in data reaching TraxLRS.

Can you check both the browser console and dblog for any error messages?

Also have you only tried the lms_h5p activity plugin or also the lms_xapi activity plugin?

🇬🇧United Kingdom catch

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

🇬🇧United Kingdom catch

Looks great! Nice to get these massive registry caches a bit smaller.

Committed/pushed to 11.x, thanks!

🇬🇧United Kingdom catch

This could use before/after screenshots in the issue summary given it's a user-facing change. Even with the test coverage I'm finding it hard to tell what the new behaviour is.

🇬🇧United Kingdom catch

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

🇬🇧United Kingdom catch

The change record is incorrect here - it needs to talk about the method being added to the interface, not the deprecation that was removed from the MR.

🇬🇧United Kingdom catch

Given @tedbow's comment, I'd prefer to have subsystem maintainer review here.

Also I think we should skip the audit operation in regular package_manager operations too - even with php-tuf performance improvements recently there is still a lot of additional http requests etc.

🇬🇧United Kingdom catch

I don't think this is straightforward, it's not clear to me whether the lack of validation should be a permanent state or whether we need a follow-up to try to add something similar to #10.

🇬🇧United Kingdom catch

Needed to fetch locally..

🇬🇧United Kingdom catch

OK interesting. I think if the DA doesn't have any objection to gitlab pages, and also doesn't want to provide some kind of alternative hosting, then that is fine then!

For me this is less a replacement for api.drupal.org and more for certain parts of the handbook. We can then remove a lot of the long prose on api.drupal.org and link from there to the new docs for higher level concepts. So https://api.drupal.org/api/drupal/core%21core.api.php/group/cache/11.x would be much shorter, with a link, but still there with the overview of interfaces/classes etc.

🇬🇧United Kingdom catch

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

🇬🇧United Kingdom catch

Also crediting @ressa for the original version of the side-by-side gif that was recreated for the front page post.

🇬🇧United Kingdom catch

@fjgarlin this is not really for code documentation as such, it's for high level documentation of subsystems.

The example I've been using (partly because I'm familiar with it, partly because it shows the issue), is the cache API.

We have:

https://www.drupal.org/docs/8/api/cache-api/cache-api

And https://api.drupal.org/api/drupal/core%21core.api.php/group/cache/11.x

Both of these are prose, the information is split between the two, as cache API maintainer I know I can submit an MR for the api.drupal.org output (although I dread doing that due to the format), but I have no idea what sign-offs are required to update the handbook documentation, it looks like the section is owned by Wim Leers, I didn't know this until I checked, I'm not sure Wim even does.

The bigger problem is the two of these shouldn't exist, we should have one canonical place for this documentation without obvious duplication. The handbook could potentially have sections about other caching things like how to set up contrib modules (redis, varnish_purge) but at the moment the documentation is split.

We can compare this to Symfony's cache API documentation: https://symfony.com/doc/current/components/cache.html

or Laravel's https://laravel.com/docs/11.x/cache

Both of these are the first result on google and it's pretty clear that's where they want you to start reading. Additionally in the side nav it's clear how to get from there to other component documentation that's equivalent.

I'm not sure there is a fixed view on the other issue that gitlab pages should be used, that feels like more of a DA decision to me anyway.

🇬🇧United Kingdom catch

Went ahead and committed this to 11.x (and kicked of a performance test job) and then 11.2.x. That gets us back to clean test runs. We can adjust again via the other issues. Thanks!

🇬🇧United Kingdom catch

OK that makes more sense.

Navigation disabled: Gin Toolbar requires toolbar and displays like before

I think this could be handled with a ::moduleExists() check and maybe hook_requirements() if neither navigation nor toolbar are enabled.

🇬🇧United Kingdom catch

Not sure I understand.

Navigation module prevents the toolbar module from rendering. So why would toolbar module be required when navigation is enabled?

🇬🇧United Kingdom catch

📌 Create the database driver for MySQLi for async queries Active just landed so this is possible to implement now.

Production build 0.71.5 2024