🇬🇧United Kingdom @longwave

Agree with #3, if someone wants to maintain a contributed module that contains BC shims for previous versions of jQuery then they are free to do so, but this doesn't fit with our BC policy for core; we are trying to minimise JavaScript and so don't want to load backward compatibility code that probably won't be used on 99% of sites.

longwave → created an issue.

Backported down to 10.5.x as a critical regression, this only affects the link and not the translated string.

Committed and pushed 054568a1f05 to 11.x and fee3dc830b3 to 11.2.x and d1db7dc0cb6 to 10.6.x and d731332ecfc to 10.5.x. Thanks!

Is it possible to get a full stack trace from where that exception happens? The update hook itself doesn't use TranslatableMarkup so it's not clear to me how or why this happens.

Thanks for working on this. As a cleanup task this is not worth backporting, committed to 11.x only.

Committed 0951bca and pushed to 11.x. Thanks!

Great work here - nice to see this finally land! Tagging as a release highlight and will publish the change record.

Committed e117838 and pushed to 11.x. Thanks!

I tried to credit everyone who provided code changes or helpful comments that moved the issue along in some way; I did not credit those who just uploaded patches, because we should be using MR workflows now. Apologies if I missed anyone.

Better title :)

Traced this back to an issue in the original config_translation module, where nl2br() was explicitly added to fix a bug: #1945184: Original value display is broken →

Therefore I think we should keep this feature? (and probably add a test for it...)

Do we know why the nl2br() was originally there? Do we need to care about representing newlines in the string correctly still?

Preprocess functions can be OOP and use DI since 11.2! https://www.drupal.org/node/3496491 →

I think catching the exception is fine if it makes the code easier to read.

I don't think we can do this without breaking existing code that expects to catch the exception.

Ideally you should avoid \Drupal::service() and use dependency injection instead.

One possible option is add a second argument to \Drupal::service() that passes through the second $invalid_behavior argument to the container, so you could do

\Drupal::service('non-existent-service', ContainerInterface::NULL_ON_INVALID_REFERENCE);

Backported to 11.2.x as an eligible bug fix. This could also go to 10.5.x/10.6.x but doesn't cherry-pick cleanly; if anyone feels strongly about that please reopen with a backport MR against 10.6.x.

Committed and pushed 970ce375723 to 11.x and 6d8c0395f8f to 11.2.x. Thanks!

I think that's all that can be tested here, thanks for working on this.

Committed 0e7d63c and pushed to 11.x. Thanks!

Committed 1459e06 and pushed to 11.x. Thanks!

The token in question is not a sensitive token, in fact it was explicitly added to prevent a security issue. It is a CSRF token designed to ensure that the user actually requested the log out function and this has not been induced by an attacker.

If you are able to perform an attack using this token, please provide further information to the security team in private by following the instructions here: https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett... → - do not post any sensitive information in this public issue.

Thanks for the fix. Backported down to 10.5.x as a docs-only bug fix.

Committed and pushed 13a1d338a4b to 11.x and 99ee2a84ebe to 11.2.x and 2039e4aaf6e to 10.6.x and 97cc774765c to 10.5.x. Thanks!

While the strtolower is incorrect, the test strings look incorrect in English now, with the capitalisation mid sentence - e.g. "Translate Taxonomy vocabulary" or "Translate Contact form"

Entities have singular labels available for this sort of thing, shouldn't we be using label_singular here instead?

#[ConfigEntityType(
  id: 'taxonomy_vocabulary',
  label: new TranslatableMarkup('Taxonomy vocabulary'),
  label_collection: new TranslatableMarkup('Taxonomy'),
  label_singular: new TranslatableMarkup('vocabulary'),
  label_plural: new TranslatableMarkup('vocabularies'),

with the button text "Translate vocabulary"

#[ConfigEntityType(
  id: 'contact_form',
  label: new TranslatableMarkup('Contact form'),
  label_collection: new TranslatableMarkup('Contact forms'),
  label_singular: new TranslatableMarkup('contact form'),
  label_plural: new TranslatableMarkup('contact forms'),

with the button text "Translate contact form"

#[ConfigEntityType(
  id: 'view',
  label: new TranslatableMarkup('View', ['context' => 'View entity type']),
  label_collection: new TranslatableMarkup('Views', ['context' => 'View entity type']),
  label_singular: new TranslatableMarkup('view', ['context' => 'View entity type']),
  label_plural: new TranslatableMarkup('views', ['context' => 'View entity type']),

with the button text "Translate view", etc.

The blocker is in.

Tried to think of downsides or problems with converting this to a service, but apart from a tiny bit of additional bloat in the container I don't think there are any issues.

Committed e19ac2e and pushed to 11.x. Thanks!

As a bug fix this is technically eligible for backport, but given this is only to fix a problem in a not-yet-committed issue I think this can remain in 11.x only.

Looks good to me, added a question about the update hook though.

!12273 is green now so this should mean no test failures once this is committed to 10.5.x and 10.6.x. Thanks for the patience and assistance with the backports here.

Committed and pushed 86faa69f084 to 10.6.x and 53f117df5f0 to 10.5.x. Thanks!

So currently this is in all supported 11.x versions but not 10.x.

✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review landed in 10.6.x and 10.5.x finally, I've rebased MR !12273 against 10.5.x and if this is green now then this means this can be committed to both those branches.

I'm also closing the 11.0.x and 10.4.x MRs as they are no longer eligible for backports.

longwave → made their first commit to this issue’s fork.

Saving credits.

Yep this was missed from 10.5.x and then not carried forward to 10.6.x when that was opened either. Too late now to backport to 11.0.x though. Thanks for keeping on top of this.

Committed and pushed efb0d9a4f17 to 10.6.x and 99be6f9bb6c to 10.5.x. Thanks!

Given the function is just

  return array_map(function ($bundle_info) {
    return $bundle_info['label'];
  }, \Drupal::service('entity_type.bundle.info')->getBundleInfo('node'));

this could be added as a convenience method to EntityTypeBundleInfo so you can call e.g.

$labels = \Drupal::service('entity_type.bundle.info')->getBundleLabels('node');

Trivial fix, the variable names do not appear anywhere else in the codebase.

Committed 4558b08 and pushed to 11.x. Thanks!

The final straggler: https://git.drupalcode.org/project/drupal/-/merge_requests/12650

Oh dammit we missed a single instance:

core/themes/olivero/css/components/header-search-wide.css:  background-image: linear-gradient(var(--color--primary-50), var(--color--primary-50)); /* Two values are needed for IE11 support. */
core/themes/olivero/css/components/header-search-wide.pcss.css:  background-image: linear-gradient(var(--color--primary-50), var(--color--primary-50)); /* Two values are needed for IE11 support. */

Nice to finally see the end of this!

Committed 1c08eaa and pushed to 11.x. Thanks!

This makes the code much easier to read, good idea.

Committed a5e7278 and pushed to 11.x. Thanks!

I could argue that helpfuLinks() should be filtered after the fact, maybe the links template could have an option to check access before rendering each link, but this works for me as a simple solution given there are only two links to check.

Committed 9a1d0b1 and pushed to 11.x. Thanks!

Thanks, a nice self contained addition that will be useful to a few people. As a new feature this is only eligible for 11.3.x, I considered backporting to 10.6.x but this doesn't meet the criteria of a critical API addition.

Committed 8326e01 and pushed to 11.x. Thanks!

I deliberately did not add this at the time we added the rest of the aliases because of the possible confusion between keyvalue and keyvalue.database as they share the interface, but you will (almost) always want keyvalue to proxy to the selected implementation so adding the alias to that makes sense so it can be injected more easily. And exactly the same applies for the expirable services too.

We can, because that will notify people via PHPStan as well as via tests or at runtime - but I suspect the PHPStan baseline will grow quite a bit.

Added similar dynamic migration messages for the EmptySource plugin and I18nQueryTrait trait and skipped the core implementations of them, they should still trigger in contrib/custom code.

Added some test coverage by removing test modules from the skip, and then expecting the deprecation in tests that use them. Let's see if anything else will fail now I copied in the other two source plugin deprecations from the original MR.

Alternative proposal in !12617 which deprecates DrupalSqlBase but then skips the deprecation notices for core extensions of that class; any other extensions in contrib will trigger the message as normal.

longwave → made their first commit to this issue’s fork.

xjm → credited longwave → .

xjm → credited longwave → .

This seems like a niche feature request and with no +1s or additional comments in nine years let's just close this out.

Sad to see you go, it was great meeting you in Kingston. Thanks for everything you have done for the project.

Committed and pushed to all actively supported branches.

Duplicate of #3161693: BigPipe corrupts HTML with multiple tags → ?

Added a suggestion to improve readability while we're here. Nitpicks welcome on the comments, or feel free to reject this if you don't think it's necessary at all.

Fixed up everything to make more sense to me at least, hopefully it is clear to everyone else as well.

As per the kernel test ones a lot of these look odd to me now, matching braces should always have the same indentation I think?

Bikeshedding: Is "bootstrapped" more descriptive than "loaded"?

It's not that the module name itself would be translated but the structure of "Module (machine_name)" might want to use symbols other than parentheses in some languages, e.g. "Module [machine_name]" or similar. Maybe it's not that important, we could probably leave it until someone complains.

Wondering if this should have been done with TranslatableMarkup, as some non-Latin character sets might want to change the structure here?

            $form['modules'][$module->getName()]['#required_by'][] = $module_name . " (" . $dependent . ")";

ie.

            $form['modules'][$module->getName()]['#required_by'][] = new TranslatableMarkup('@module (@machine_name)', ['@module' => $module_name, '@machine_name' => $dependent]);

While this is largely subjective I feel there's definitely some imbalances going on here.. I feel that starting/ending braces etc should be at the same indentation level at least, and there's some cases that can be rearranged to make them easier to read. I added some sample suggestions but there are other similar cases that should also be considered.

xjm → credited longwave → .

xjm → credited longwave → .

Start a fresh installation of Drupal and try to reproduce it there. If you can't, try adding themes or modules that you are using until you find which one causes the problem.

When we upgraded to PHPUnit 11 we didn't spot the major version upgrade of sebastian/diff in drupal/core-recommended. I think we are confident that core works with both versions of the diff library so we could expand the constraints to allow both versions; this in turn means core-recommended users should be able to use both PHPUnit 10 and 11.

As the methods that use the constants are protected shouldn't the constants also be protected? Or are we saying you shouldn't actually be overriding those methods?

composer why-not drupal/core 11.2.0 may explain the reason more clearly.

In 📌 Deprecate TestDiscovery test file scanning, use PHPUnit API instead Active it appears that core-recommended bumped the major version of sebastian/diff, we did not catch this nor pick it up in the release notes

-        "sebastian/diff": "~5.1.1",
+        "sebastian/diff": "~6.0.2",

Duplicate of 📌 11.2.0 frontpage post Active

Do you have any other CKEditor related contrib modules installed?

jurgenhaas → credited longwave → .

xjm → credited longwave → .

Should the route move to rest.module? We could either enable it by default only if rest.module is enabled, or add it as a config option there?

Second time lucky? Changed my mind about backporting, not worth going to 11.2.x/10.5.x here now.

Committed and pushed to 11.x and 10.6.x.

Some nits/suggestions. The sample regex-like URL confused me so I think we should add a comment explaining it.

Hm, over in 📌 Add validation constraints to contact.settings Active we just committed a bunch of changes with

      constraints:
        Range:
          min: 0

Is PositiveOrZero preferred? If so, why? There is no discussion of adding it in this issue. We should be consistent, in which case should we update other cases of Range with a minimum of 0?

A relatively straightforward one here, the slow march to full config validation continues.

Committed e9ee357 and pushed to 11.x. Thanks!

Looks good, one question about the placement of IsCommentableConstraint.

Two nits, I think "fragment" is a more recognisable and well-defined term than "snippet".

Committed a8d4ed9 and pushed to 11.x. Thanks!

As a new feature in the test framework this isn't eligible for backport to 11.2.x and I'm not sure it's worth copying to 10.6.x either, please reopen if you disagree.

This change makes sense to me and simplifies the code path a bit while making things more secure, so good all round.

Committed 56fe980 and pushed to 11.x. Thanks!

As this is a minor behaviour change, but also a security improvement, I'm on the fence somewhat about backporting to 11.2.x. This is definitely eligible for 10.6.x (and perhaps 10.5.x if we also commit to 11.2.x), but the MR doesn't apply, so marking PTBP so we can decide what to do next here.

xjm → credited longwave → .

xjm → credited longwave → .

longwave → created an issue.

xjm → credited longwave → .

longwave → made their first commit to this issue’s fork.

Added release note snippet.

It's not that important to backport, but I guess we could to keep everything in sync?

@godotislate actual changes to the standards are usually from stylelint-config-standard's major version updates, e.g. v38 added color-function-alias-notation: "without-alpha" rule which enforced the rgba to rgb change: https://github.com/stylelint/stylelint-config-standard/releases

Updated the release note a bit. Not sure we need a change record to document what an upstream dependency did?

Thanks, that's the exact fix I was thinking of. However, this could do with a test to prevent regressions.

Oh it's only in the test, I thought there was another compatibility layer somewhere but maybe that's in XB.

In which case let's just ship this and deal with Coder and PHPStan elsewhere.

@catch do we want to remove that FC/BC layer? We still allow 5.2 || ^6.3 in composer.json.

Let's defer PHPStan to 📌 Update PHPStan to 2.1.17 Active

Coder is being handled in 📌 Update Coder to 8.3.28 Active , and we need to stay on PHPUnit 10 and Lexer 2 for now although we allow newer versions:

$ composer update --with=drupal/coder:8.3.26 --with=phpunit/phpunit:^10 --with=doctrine/lexer:^2
...

$ composer-lock-diff --no-links
+-----------------------------------+---------+---------+
| Dev Changes                       | From    | To      |
+-----------------------------------+---------+---------+
| brick/math                        | 0.12.3  | 0.13.1  |
| composer/ca-bundle                | 1.5.6   | 1.5.7   |
| composer/composer                 | 2.8.6   | 2.8.9   |
| composer/spdx-licenses            | 1.5.8   | 1.5.9   |
| google/protobuf                   | v4.30.2 | v4.31.1 |
| justinrainbow/json-schema         | 5.3.0   | 6.4.2   |
| mglaman/phpstan-drupal            | 2.0.5   | 2.0.7   |
| nikic/php-parser                  | v5.4.0  | v5.5.0  |
| open-telemetry/api                | 1.2.3   | 1.3.0   |
| open-telemetry/exporter-otlp      | 1.2.1   | 1.3.1   |
| open-telemetry/sdk                | 1.3.0   | 1.5.0   |
| phpspec/prophecy-phpunit          | v2.3.0  | v2.4.0  |
| phpstan/phpstan                   | 2.1.14  | 2.1.17  |
| phpstan/phpstan-deprecation-rules | 2.0.2   | 2.0.3   |
| ramsey/uuid                       | 4.7.6   | 4.8.1   |
| slevomat/coding-standard          | 8.18.0  | 8.19.1  |
| squizlabs/php_codesniffer         | 3.12.2  | 3.13.0  |
| marc-mabe/php-enum                | NEW     | v4.7.1  |
+-----------------------------------+---------+---------+

longwave → made their first commit to this issue’s fork.

Rebased.

I think the icon break is the biggest one so explicitly called that out with a link to the CR, otherwise not sure which ones are important so added a catch-all and linked to the CKE upgrade notes.

There is an additional hard problem in the case of interdependent entities: if entity type A depends on entity type B, and both receive some kind of structure update using this mechanism, which order do the updates run in? Does the update code for entity type A have to deal with the old or new structure of entity type B? Perhaps this is a hypothetical issue for now but as @berdir mentioned contrib I am sure we will run into this case sooner or later, if what we are effectively trying to here is provide long term BC for data structures.

Updated all dependencies for minor/patch level releases.

yarn audit reduced from 6 to 2 warnings, the two remaining are Nightwatch related where we are stuck on an older version.

longwave → made their first commit to this issue’s fork.

NW for #11/#12. Thanks @godotislate for helping out here!

I actually think the core-recipe-unpack thing was correct.. it's in 11.2.x lockfile and should have been added to 11.x as well, otherwise I get this on install after switching branches:

Package operations: 0 installs, 3 updates, 1 removal
  - Removing drupal/core-recipe-unpack (11.2.x-dev)
  - Upgrading drupal/core-project-message (11.2.x-dev => 11.x-dev): Source already present
  - Upgrading drupal/core-vendor-hardening (11.2.x-dev => 11.x-dev): Source already present
  - Upgrading drupal/core (11.2.x-dev => 11.x-dev): Source already present

We need separate MRs for both 11.x and 11.2.x as this has to land in both but the lockfile causes conflicts so we can't reuse the same patch.

Updated to v45.2.0.

Thanks for working on this. I've reviewed the MR and the changes all make sense, not too worried about any of the test changes.

For the icons we have some choices: I think this initial BC layer is a good idea. I think we should also issue a deprecation from the PHP side if we detect an old icon name; this way we can remove the BC layer in the future. Alternatively we could also move the entire BC layer to the PHP side, and it doesn't look too hard to support all the icon names they changed, there aren't actually that many.

On the other hand, we are running up against the rc1 deadlines, so the quicker we can just get this in the better; maybe we could add the deprecation and improved BC layer in a followup.

NW for updating to v45.2.0.

https://github.com/ckeditor/ckeditor5/releases/tag/v45.2.0 is out, we should try to bump to that I guess.

No, the primary reason is that it's security through obscurity: hiding version numbers achieves nothing. Almost all attacks are automated, most attackers won't even check the version number and will blindly try anyway, and those that do check will fingerprint the file, rather than interpreting the query string.