We should get this in ASAP and ship it with 11.0.0.
@woldtwerk if you want to propose changing our JS standards for something else please open an issue in https://www.drupal.org/project/issues/coding_standards → with some more details as to what the change is and why you think it is better.
Yep let's land this for now and fix in a followup.
Looks good to me, makes sense to use the base class. The two original patches were committed within a month of each other, I guess we just never considered it at the time.
Yup this can be closed as fixed now, the rc1 issue is already closed. I tried to credit everyone who contributed to keeping the issue summary up to date.
The Drupal 12 top level meta is also open: 🌱 [12.x] [meta] Release Drupal 12 in 2026 Active so removing the followup tag for that.
I'm slightly amazed that it was only another call to execute() that needed fixing, but the run is green.
Last run was looking promising until it appeared to hang after installProfileTest: https://git.drupalcode.org/project/drupal/-/jobs/2214554
api.execute() takes the callback as the third argument instead of the second now, it seems.
Locally I bumped to Nightwatch 3.7.0 which works OK with the new Selenium setup and latest Chrome, but tests appear to crash and then the Selenium server no longer responds:
$ yarn test:nightwatch tests/Drupal/Nightwatch/Tests/loginTest.js
[Tests/Login Test] Test Suite
──────────────────────────────────────────────────────────
ℹ Connected to selenium on port 4444 (505ms).
Using: chrome-headless-shell (126.0.6478.114) on LINUX.
ℹ Loaded url http://ddev-drupal8-web in 446ms
Running Test login:
───────────────────────────────────────────────────────────────────────────────────────────────────
ℹ Loaded url http://ddev-drupal8-web/user/reset/1/1721811628/FNZSmygKgxaar4E9AmDAlZ75Q61bU4ew0iwBaov3XUA/login
in 440ms
ℹ Loaded url http://ddev-drupal8-web/admin/people/roles/add in 133ms
✔ Expected element <.user-role-form .machine-name-value> to be visible in 2000ms (36ms)
✔ Testing if element <[data-drupal-messages]> contains text 'Role xt4aw4gms9 has been added.' (24ms)
ℹ Loaded url http://ddev-drupal8-web/admin/people/permissions in 160ms
✔ Element <table.permissions> was visible after 34 milliseconds.
✔ Testing if element <[data-drupal-messages]> contains text 'The changes have been saved.' (22ms)
ℹ Loaded url http://ddev-drupal8-web/user/logout/confirm in 69ms
ℹ Loaded url http://ddev-drupal8-web/user/reset/1/1721811630/RH7RHeQ6JGfOHJ7RGMIXnuu3wYDMoU3KEqDnBTZrHfI/login
in 83ms
ℹ Loaded url http://ddev-drupal8-web/admin/people/create in 135ms
✔ User "user" was created successfully (34ms)
ℹ Loaded url http://ddev-drupal8-web/user/logout/confirm in 47ms
ℹ Loaded url http://ddev-drupal8-web/user/login in 54ms
✔ Passed [equal]: The user "user" was logged in.
ℹ Loaded url http://ddev-drupal8-web/admin/reports in 76ms
✔ Element <body> was visible after 30 milliseconds.
✔ Testing if element <h1> contains text 'Reports' (32ms)
$
I was expecting further output here, if I run this on the older version of Nightwatch it ends as follows:
✔ Testing if element <h1> contains text 'Reports' (29ms)
✔ Ensuring no deprecation errors have been triggered (10ms)
✨ PASSED. 9 assertions. (3.879s)
Wrote HTML report file to: /var/www/html/drupal/core/reports/nightwatch/nightwatch-html-report/index.html
$
If I then run the test again it hangs:
$ yarn test:nightwatch tests/Drupal/Nightwatch/Tests/loginTest.js
[Tests/Login Test] Test Suite
──────────────────────────────────────────────────────────
⠴ Connecting to selenium on port 4444...
Restarting the Selenium container fixes it.
Will push this branch up anyway but not sure what's happened here at all.
📌 Use selenium/standalone-chrome instead of our chromedriver image Needs work landed which gives us a newer Selenium/Chrome to talk to and which may solve the issues we were seeing here.
I don't think there is one, it's kinda case by case - it's suitable when the individual tests don't do much work but also we must ensure that combining them doesn't cause side effects where data from an earlier test leaks into a later one.
Makes sense to me, as long as it doesn't affect the timings why reinstall over and over. Backported down to 10.3.x as a test-only change.
Committed and pushed 24b7c3ffa3 to 11.x and 64e80c8d84 to 11.0.x and 12448d24bc to 10.4.x and 816ac01be9 to 10.3.x. Thanks!
This was solved in 🐛 Never show a "Not supported!" version as "Recommended" Needs work . I just patched my 9.5 site with the changes from that issue and get a better message:
So if this is fixed in newer versions and there is nothing now that can be done in 9.5, I think this can be closed as duplicate?
I updated the link in #11. I think this will get slightly more use than "Data model changes" which I can't remember the last time I saw filled in.
I also find it somewhat funny that we are known as a CMS for structured content and yet here we are, writing structured content into a free text body field, when we raise issues against our own software - if we used proper fields we could have help text, show different fields for bugs vs tasks, etc.
longwave → made their first commit to this issue’s fork.
Backported to 10.4.x. Not eligible for 10.3.x or 11.0.x as it's a translatable string change.
Committed and pushed 269e502c55 to 11.x and d3188f36ff to 10.4.x. Thanks!
Backported to 10.4.x to keep things in sync, I doubt this would break anything but I don't see the need to backport to 11.0.x or 10.3.x.
Committed and pushed e3b52e2f3f to 11.x and 511d6fe569 to 10.4.x. Thanks!
Committed to 11.1.x and 10.4.x, this isn't a bug fix or critical task so not eligible for backport to 11.0.x or 10.3.x
Committed and pushed ac88e9a4d8 to 11.x and 6fc96204f7 to 10.4.x. Thanks!
There is a mismatch in the return value in the new method, and it made me realise the new method is doing two jobs; should we split it into two methods?
Good find. Backported down to 10.3.x as a bug fix that should affect approximately no-one.
Committed and pushed ca4903b79b to 11.x and 3d6eabd010 to 11.0.x and 7d18afd9ef to 10.4.x and 7c9adb2db4 to 10.3.x. Thanks!
FWIW for sites where developers are purely in control of the strings I use the $settings option, or if the customer wants to modify strings themselves you can still install locale and interface translation modules, but only have English language installed, and let them edit the English strings in the UI.
The string is wrapped in Drupal.t() so you should be able to use the standard translation mechanisms to alter the message already? We don't usually provide explicit settings for user-facing strings for this reason - the translation system is preferred, and it also handles the case of multiple languages by default (which I'm not sure the proposed solution does?)
We could still consider improving the default message in this issue.
Alright, let's ship this - tests are green, and the test and configuration changes are the minimum required to enable W3C mode. This then unlocks some future improvements including hopefully being able to upgrade Nightwatch to a more recent version.
Also just run into this on a small site that was way behind and has just been updated to 9.5.11, it's also recommending going to 11.0.0-rc1 or 10.2.2 as a security update, and doesn't mention 10.3 at all.
Yep this should have been postponed initially, but this is eligible to work on now 🐛 Disabled text formats can't be seen in the GUI Fixed has landed.
Committed to 11.1.x and backported to 10.4.x as a usability bug fix. Not eligible for 11.0.x or 10.3.x as it changes the user interface.
Committed and pushed afdee270cb to 11.x and c0a5216300 to 10.4.x. Thanks!
The suggested followup already exists, will un-postpone it after submitting this comment.
Done.
Note that 🐛 views.settings config object should not be used to cache list of available plugins Active proposes removing this config entirely, is effort better spent over there?
There are a few merge conflicts and some missing docblocks but otherwise to me this is ready to go - still needs framework manager signoff though.
Added to the default issue template and updated the documentation as per the examples in the IS. I also added an ID to the new h3 in the documentation so it can be linked if required.
Added introduced terminology section 📌 Add "Introduced terminology" section to the "Bare issue summary template" default heading for new issues Fixed
Backported down to 10.3.x as a test-only change.
Committed and pushed dca3ac8cbe to 11.x and 4420aa8ee9 to 11.0.x and cb52e79765 to 10.4.x and bcf87a8b80 to 10.3.x. Thanks!
Decided to commit this to 11.1.x and 10.4.x only as there is a chance of it being vaguely disruptive to existing sites that are using RSS and not necessarily expecting CDATA in their output here, given we wrote a change record it makes sense to put it in a minor release only.
Still, great to see a 20+ year old bug finally fixed, and in such a clean way given we can just add the event subscriber to modify RSS responses.
Committed and pushed eba56b0fd7 to 11.x and ebc8e0639e to 10.4.x. Thanks!
In the interests of not holding up a 20+ year old issue any longer, I fixed the nit and set it back to RTBC and will commit later if the tests pass.
I think this solution is really clean, just one nit about an unused argument.
Fixing issue status and credits after crossposting with the bot.
Backported down to 10.3.x as a valid bug fix, although this is a very minor behaviour change it makes things better and I see no reason to keep it to major releases only.
Committed and pushed c29768f406 to 11.x and 1e5f032e26 to 11.0.x and bc8de78fe2 to 10.4.x and 5be98c43a8 to 10.3.x. Thanks!
Committed and pushed afce6da313 to 11.x and e8d874f4db to 11.0.x and b9c8018c12 to 10.4.x and 32075e1a5f to 10.3.x. Thanks!
catch → credited longwave → .
@dww I think those are OK to leave as deprecated in 11.1 for removal in 12, we are removing the internals as well in 11.1 as we don't need to support that any more - but I doubt anyone else is calling inside the updater code in 10.4 and needs notifying now so not worth backporting down to 10.4.
As far as I can see nothing else is left to do here, so marking RTBC.
Opened 📌 Add helper method to generate HTML placeholders Active to simplify the code here.
longwave → created an issue.
Let's update the CR for Nightwatch and also add a followup to investigate Selenium switches, otherwise this is ready to go.
Apache and nginx still also log IP addresses by default. This data is often useful, accessible only by administrators (and in the syslog case is inaccessible from Drupal itself) and can't be recreated after the fact in the case of an incident, to me this is won't fix.
Looks good, nothing more to add.
Looks good to me, plus gives a tiny performance improvement in every page load given we are one event listener lighter.
It would be really helpful if one of the reporters in this issue can test the patch but in the meantime this is RTBC.
Committed and pushed 63bc78c2d4 to 11.x and ee32e930be to 11.0.x and a8bb272ff9 to 10.4.x and 9320092df7 to 10.3.x. Thanks!
Backported down to 10.3.x to keep things in sync.
Committed and pushed 745a0e25dc to 11.x and 42b28e4855 to 11.0.x and b74f2b159f to 10.4.x and f59d1e0cac to 10.3.x. Thanks!
catch → credited longwave → .
Maybe we just copy the code to both methods for now, and consider unifying it later?
Added some nits/comments to the MR.
If we're just moving Nightwatch entirely to w3c mode then we can drop the separate selenium environment and just turn on the w3c switch in the default environment.
Added some code comments, I think the code could be cleaner and there is an open question about the cache key.
larowlan → credited longwave → .
We should implement #16 in code as a status report message if the module is enabled but can safely be disabled.
Doesn't seem to be able to run more than 150 or so without crashing the entire runner, but we do now have 500 runs or so with no actual failures.
500x job failed due to a GitLab runner system failure, trying again at https://git.drupalcode.org/project/drupal/-/jobs/2161641
Hopefully this one has gone away either with the move to GitLab or some other fix we have made in the past two years.
https://git.drupalcode.org/project/drupal/-/jobs/2161099 ran 100x with no fails.
https://git.drupalcode.org/project/drupal/-/jobs/2161218 running 500x, if that doesn't fail either then I suggest we revert the skip.
longwave → changed the visibility of the branch 3180410-longwave-patch-testing to hidden.
longwave → changed the visibility of the branch 11.x to hidden.
longwave → changed the visibility of the branch symfony-7 to hidden.
longwave → changed the visibility of the branch cspell-test to hidden.
longwave → made their first commit to this issue’s fork.
Agree this needs tests. What is the use case? The Symfony link implies that if you use the @? syntax then NULL is passed if a service is not found, but I don't see what this has to do with the reverse container.
Opened 📌 Update to jQuery 4.0.0-beta2 Active
longwave → created an issue.
Both MRs look great to me.
Thanks again @dww. The 11.x MR looks great and ready to go to me.
Some notes on the backport MR:
- As per #26 let's add a trigger_error deprecation to the
UpdateManagerInstallform. - Let's also backport the help text and comment changes from system.module and module.api.php to keep them in sync.
- I think we should also add a deprecation notice to
update_authorize_run_install()just in case - deprecated in 10.4 for removal in 11.1 I guess.
The change record needs to be written but otherwise the tests are green so marking NR, hopefully someone else familiar with Chrome testing can take a look.
* cache.name_of_bin:
* class: Drupal\Core\Cache\CacheBackendInterface
* tags:
* - { name: cache.bin, bin: name_of_bin }
* factory: ['@cache_factory', 'get']
* arguments: [name_of_bin]
So now we have to specify the bin name twice, in the tag and as the factory argument? Can we avoid this?
I did a minor rearranging of the code to keep the teaser setup and its deprecation together, otherwise this looks fine to me so RTBC.
longwave → made their first commit to this issue’s fork.
Moving to the API project that generates api.drupal.org.
This looks great! Happy to see this all finally being removed.
The MR removes some references to system.theme_install but that still exists (update.theme_install is removed, though).
Also this route reference still exists in \Drupal\Core\Updater\Module::postInstallTasks():
$default_options + [
'#url' => Url::fromRoute('update.module_install'),
'#title' => t('Add another module'),
],
bbrala → credited longwave → .
Yeah I'm not sure of the benefit of shipping config.yaml because the auto configuration has just worked for me.
What might be useful though is shipping a docker-compose partial config for the Chrome container required to run JS tests and the related environment variables.
File cache was probably much more important when we were parsing for annotations? But maybe we can move it to that layer and drop it for attributes, though it would be good to do some benchmarking if possible.
Ugh, yeah, there is caching bug there, but not sure how to work around it.
Also, how about logging something if we encounter an exception, instead of swallowing it - it might help developers if they have typo'd something or similar that prevents discovery from working?
It should not be required to enable state caching - that is an opt-in feature added in Drupal 10.3 and enabled by default in Drupal 11. However if this does make a noticeable difference here, that is an interesting data point that we should look into further.
Doesn't seem like there's much we can do here until the upstream packages are all compatible too.
Thanks @Feuerwagen, as the maintainer is very responsive let's wait a few days for the security policy to hopefully be adopted and then we can move forward here.
Also fixing obsolete link in the IS.
Well the code just does this:
['id' => $id, 'content' => $content] = $this->parseClass($class, $fileinfo);
if ($id) {
$definitions[$id] = $content;
// Explicitly serialize this to create a new object instance.
$this->fileCache->set($fileinfo->getPathName(), ['id' => $id, 'content' => serialize($content)]);
}
else {
// Store a NULL object, so the file is not parsed again.
$this->fileCache->set($fileinfo->getPathName(), [NULL]);
}
So really it's just replacing that $id check with a catch? Or, we also allow parseClass to return just NULL directly instead of an array of NULLs?
Failed as expected; postponing on that issue, although we could merge the two.
longwave → created an issue.
I think this makes sense and I can't see a simpler or better way to do it.
However let's open a followup to discuss that return value of parseClass() because I agree the structure with NULLs is strange, perhaps it should throw an exception it probably should throw an catchable exception in the failure case instead? The reason we do it is:
['id' => $id, 'content' => $content] = $this->parseClass($class, $fileinfo);
so the return value is always expected in that structure. I think this is all internal enough that we can change it without BC implications, but the followup can help us decide that.
GenerateThemeTest should fail here even on mysql because of 🐛 GenerateThemeTest::testContribStarterkitDevSnapshotWithGitNotInstalled fails on sqlite Needs review
longwave → created an issue.
Annoyingly as this didn't make 11.0.0-rc1 we can't just commit this as-is. Discussed with @xjm and although we only provide best-effort BC in tests there are some base classes and common traits touched here that I think we should avoid changing except in a major release, ie they will have to wait for 12.x now.
Some files that I think we should revert here are:
core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
core/modules/migrate/tests/src/Kernel/MigrateTestBase.php
core/modules/rest/tests/src/Functional/ResourceTestBase.php
core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php
core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php
core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
core/tests/Drupal/KernelTests/AssertContentTrait.php
core/tests/Drupal/KernelTests/KernelTestBase.php
core/tests/Drupal/Tests/BrowserTestBase.php
core/tests/Drupal/Tests/UiHelperTrait.php
These are just the ones that I know are widely used as base classes/traits, there may be more, although there's also a tradeoff in considering which methods are really likely to be overridden in subclasses.
Committed and pushed 3ef0e0a247 to 11.x and cfb5a31dca to 11.0.x. Thanks!
Doesn't cleanly cherry-pick to 10.4.x/10.3.x, not sure it's worth the effort.
I assume this only happens on multilingual sites but are there concrete steps to reproduce? e.g. if you have a multilingual site and clear cache then visit the homepage, does it happen then? If not, can you work out the sequence of events that cause it?
Downgrading to "major" as this is not a site-down event and it appears from reports the site mostly works, it is only the homepage that is affected and even then not all the time.
CronRunTrait has the cronRun method, here we have only changed the runCron method which is different. There is no open MR and so the tests haven't even run yet.