UK
Account created on 22 February 2008, over 17 years ago
#

Merge Requests

More

Recent comments

🇬🇧United Kingdom longwave UK

I think also we should add attribute support, as autowiring works elsewhere with attributes users will expect it to be the same here.

🇬🇧United Kingdom longwave UK

This is purely a frontend decision.

🇬🇧United Kingdom longwave UK

Added a question about private Vs protected, I think we perhaps have a valid case for a private property here.

🇬🇧United Kingdom longwave UK

Thank you @andypost.

🇬🇧United Kingdom longwave UK

> Every even-numbered .4 release is designated as an LTS.

Where does it say this? On the contrary it appears they call every .4 release an LTS?

And this still makes little difference to Drupal:

Drupal 10 will be EOL in December 2026. Symfony 6.4 will be EOL in November 2027.

We haven't announced an exact date yet but Drupal 11 is likely to be EOL in December 2028. Symfony 7.4 will be EOL in November 2029.

Drupal 12 dates haven't been confirmed, but neither has an end date been set for Symfony 8.

Even if we extended for another eleven months to meet Symfony, we also have to consider our other dependencies such as CKEditor, who won't necessarily make the same promises.

This is all information that hasn't changed since the last issue where you asked for the EOL to be extended, so I'm not sure of the point of rehashing it all again here; you will get the same answer.

🇬🇧United Kingdom longwave UK

Before we could do this we would need confirmation from Symfony of which versions will be LTS, also many years in advance; can you link to any Symfony policy statements where this has been announced? If not, I don't see how we can promise anything up front.

🇬🇧United Kingdom longwave UK

Wondering if we can mark them as deprecated in user.routing.yml then alter the routes in rest.module (if it's installed) to remove the deprecation again. Then in Drupal 12 we move them directly to rest.routing.yml.

When do the deprecations get triggered for deprecated routes? It's not clear to me how people will discover this change if they are using the endpoints externally.

🇬🇧United Kingdom longwave UK

Do we need to change the paths here? It would be better if users of the endpoints didn't have to update their code.

🇬🇧United Kingdom longwave UK

@catch I think 📌 Move JSON:API validation to a feature flag or separate module Active is a good enough followup already?

🇬🇧United Kingdom longwave UK

@andypost see #3552765-7: Update Composer dependencies for 11.3.0 for reasons why masterminds/html5 breaks tests, probably a duplicate of that issue?

🇬🇧United Kingdom longwave UK
🇬🇧United Kingdom longwave UK

Yes, to put it in simple terms: turn the deprecations into assertions.

🇬🇧United Kingdom longwave UK

CKEditor5 image test failures were due to https://github.com/Masterminds/html5-php/pull/258 - the HTML has two width attributes, and the rule in HTML5 is to take the first value and ignore the latter; this was incorrect in masterminds/html5 until 2.10.

🇬🇧United Kingdom longwave UK

Honestly I think we should just drop that library without deprecation or replacement, it's a small amount of CSS that is very specific to this module and I don't see why we need to go through the deprecation dance for it.

🇬🇧United Kingdom longwave UK

The only major change is doctrine/lexer has gone from 2 to 3. We could still pin at 2, but we added compatibility for 3 back in 📌 [PP-1] Make doctrine/lexer:^3.0 compatible with \Drupal\Component\Annotation\Doctrine. Active so probably it's time to upgrade. Otherwise the usual round of minor and patch version bumps.

Two things we can't upgrade to yet are:

+----------------------------------+---------+---------+
| Production Changes               | From    | To      |
+----------------------------------+---------+---------+
| composer/semver                  | 3.4.3   | 3.4.4   |
| doctrine/deprecations            | 1.1.5   | REMOVED |
| doctrine/lexer                   | 2.1.1   | 3.0.1   |
| masterminds/html5                | 2.9.0   | 2.10.0  |
| pear/archive_tar                 | 1.5.0   | 1.6.0   |
| symfony/console                  | v7.3.0  | v7.3.4  |
| symfony/dependency-injection     | v7.3.0  | v7.3.4  |
| symfony/error-handler            | v7.3.0  | v7.3.4  |
| symfony/event-dispatcher         | v7.3.0  | v7.3.3  |
| symfony/filesystem               | v7.3.0  | v7.3.2  |
| symfony/finder                   | v7.3.0  | v7.3.2  |
| symfony/http-foundation          | v7.3.0  | v7.3.4  |
| symfony/http-kernel              | v7.3.0  | v7.3.4  |
| symfony/mailer                   | v7.3.0  | v7.3.4  |
| symfony/mime                     | v7.3.0  | v7.3.4  |
| symfony/polyfill-ctype           | v1.32.0 | v1.33.0 |
| symfony/polyfill-iconv           | v1.32.0 | v1.33.0 |
| symfony/polyfill-intl-grapheme   | v1.32.0 | v1.33.0 |
| symfony/polyfill-intl-idn        | v1.32.0 | v1.33.0 |
| symfony/polyfill-intl-normalizer | v1.32.0 | v1.33.0 |
| symfony/polyfill-mbstring        | v1.32.0 | v1.33.0 |
| symfony/polyfill-php84           | v1.32.0 | v1.33.0 |
| symfony/polyfill-php85           | v1.32.0 | v1.33.0 |
| symfony/process                  | v7.3.0  | v7.3.4  |
| symfony/routing                  | v7.3.0  | v7.3.4  |
| symfony/serializer               | v7.3.0  | v7.3.4  |
| symfony/string                   | v7.3.0  | v7.3.4  |
| symfony/validator                | v7.3.0  | v7.3.4  |
| symfony/var-dumper               | v7.3.0  | v7.3.4  |
| symfony/var-exporter             | v7.3.0  | v7.3.4  |
| symfony/yaml                     | v7.3.0  | v7.3.3  |
+----------------------------------+---------+---------+

+------------------------------------------------+---------+---------+
| Dev Changes                                    | From    | To      |
+------------------------------------------------+---------+---------+
| brick/math                                     | 0.12.3  | 0.14.0  |
| composer/ca-bundle                             | 1.5.6   | 1.5.8   |
| composer/class-map-generator                   | 1.6.1   | 1.6.2   |
| composer/composer                              | 2.8.9   | 2.8.12  |
| composer/spdx-licenses                         | 1.5.8   | 1.5.9   |
| dealerdirect/phpcodesniffer-composer-installer | v1.0.0  | v1.1.2  |
| google/protobuf                                | v4.31.1 | v4.33.0 |
| justinrainbow/json-schema                      | 6.5.2   | 6.6.0   |
| micheh/phpcs-gitlab                            | 2.0.0   | 2.1.0   |
| open-telemetry/api                             | 1.3.0   | 1.7.0   |
| open-telemetry/context                         | 1.2.1   | 1.4.0   |
| open-telemetry/exporter-otlp                   | 1.3.1   | 1.3.2   |
| open-telemetry/gen-otlp-protobuf               | 1.5.0   | 1.8.0   |
| open-telemetry/sdk                             | 1.5.0   | 1.9.0   |
| open-telemetry/sem-conv                        | 1.32.0  | 1.37.0  |
| phpspec/prophecy-phpunit                       | v2.3.0  | v2.4.0  |
| phpstan/phpdoc-parser                          | 2.1.0   | 2.3.0   |
| phpstan/phpstan                                | 2.1.26  | 2.1.31  |
| ramsey/uuid                                    | 4.8.1   | 4.9.1   |
| sirbrillig/phpcs-variable-analysis             | v2.12.0 | v2.13.0 |
| slevomat/coding-standard                       | 8.18.1  | 8.22.1  |
| squizlabs/php_codesniffer                      | 3.13.0  | 3.13.4  |
| symfony/browser-kit                            | v7.3.0  | v7.3.2  |
| symfony/dom-crawler                            | v7.3.0  | v7.3.3  |
| symfony/lock                                   | v7.3.0  | v7.3.4  |
| tbachert/spi                                   | v1.0.3  | v1.0.5  |
| doctrine/deprecations                          | NEW     | 1.1.5   |
+------------------------------------------------+---------+---------+
🇬🇧United Kingdom longwave UK

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

🇬🇧United Kingdom longwave UK

Is there a use case for using the REST login route without having the REST module installed? It seems like a niche thing to do, we could just document in the release notes that you have to enable the REST module now if you are using this.

🇬🇧United Kingdom longwave UK

Proof of concept implementation in MR!13509 - SystemController::themesPage() has two dependencies injected directly.

🇬🇧United Kingdom longwave UK

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

🇬🇧United Kingdom longwave UK

To me this is works as designed at this stage. Config now generally lives outside the document root, private files should also for the most secure setup. Changing this seems like a lot of hassle for no real benefit.

🇬🇧United Kingdom longwave UK

Added a BC layer for AnnotationException, and updated the change record.

I am not sure we should provide BC for anything else here. If you were depending on doctrine/annotations, and you required it in composer.json: your code will still work. If you were depending on doctrine/annotations as a transitive dependency of core, I am not sure that is core's responsibility to fix.

🇬🇧United Kingdom longwave UK

Maybe we need a separate map object where we store the arguments once against the class name. Which when I write it out, sounds quite like a container!

🇬🇧United Kingdom longwave UK

Can't reproduce those three fails locally, all those tests pass for me.

🇬🇧United Kingdom longwave UK

Needs tests, but MR!13493 should improve performance by handling reflection at discovery time for plugins autowired via PluginBase::create().

🇬🇧United Kingdom longwave UK

Retitling this as I think we can narrow the scope a bit.

🇬🇧United Kingdom longwave UK

Posted the patch as an MR so the CI can run.

🇬🇧United Kingdom longwave UK

Looking at the patch this is just a minor hardening for the case where the settings array isn't the expected shape, given this prevents an edge case failure in the upgrade path I think we should just go ahead and commit this.

🇬🇧United Kingdom longwave UK

I think they can be done independently? What I'm kinda proposing here is splitting up the reflection and instantiation parts so they are done at different times, but as long as both parts understand about closures, it doesn't matter whether the reflection is done at discovery or runtime.

🇬🇧United Kingdom longwave UK

📌 Allow plugin service wiring via constructor parameter attributes Needs work landed, which implements this at runtime instead of discovery time. I think we can continue here with adding the reflection step at discovery time and using that data if it's available instead of needing reflection at runtime.

🇬🇧United Kingdom longwave UK

Now we've done this I think we can also simplify 📌 Allow plugin service wiring via constructor parameter attributes Needs work to carry out the reflection at discovery time and store the autowiring configuration in the plugin definition, then we won't need reflection at runtime.

🇬🇧United Kingdom longwave UK

@nicxvan it's only used if the plugin explicitly implements ContainerFactoryPluginInterface, otherwise the factory method is never called, if that helps.

🇬🇧United Kingdom longwave UK

Thanks! Opened 📌 Remove manual create() method from plugins Active as a followup so we can remove a bunch of code from core.

🇬🇧United Kingdom longwave UK

longwave created an issue.

🇬🇧United Kingdom longwave UK

If we're implementing this as best practices only that is fine - but we should add a test to ensure any new info.yml added to core also conforms to the same format.

🇬🇧United Kingdom longwave UK

Not sure this really needed a change record, but I'll publish it seeing as there is one.

Committed 3e9f284 and pushed to 11.x. Thanks!

🇬🇧United Kingdom longwave UK

Committed 29f9755 and pushed to 11.x. Thanks!

🇬🇧United Kingdom longwave UK

I don't think we need the followup tag any more.

If we are going to enforce this however, can we add a unit test that ensures all core .info.yml files have a prefix?

🇬🇧United Kingdom longwave UK

Committed 84b7eda and pushed to 11.x. Thanks!

🇬🇧United Kingdom longwave UK

Change record links are invalid.

Also not very happy about the phpcs:ignore tags as they make the docs super hard to read - more meta tags than useful information! - but not sure what we can do about that.

🇬🇧United Kingdom longwave UK

Backported down to 10.5.x as an eligible bug fix.

Committed and pushed a7b619c813e to 11.x and acd99cccc62 to 11.2.x and cb93ef7747a to 10.6.x and ad75505b110 to 10.5.x. Thanks!

🇬🇧United Kingdom longwave UK

Committed 727dc95 and pushed to 11.x. Thanks!

🇬🇧United Kingdom longwave UK

Kinda agree with @alexpott here, it's odd to be triggering any of these as deprecations? Given 📌 Expose API to sort arbitrary config arrays Needs work has been open for four years already I'm wary of adding something "temporary" to the deprecation skips which will likely not turn out to be not that temporary.

+    if ($expected_validated_value !== $value_to_set) {
+      @trigger_error(sprintf('The `type: %s` config schema type uses magical casting when validating.', $key), E_USER_DEPRECATED);
+    }

This is apparently not triggered, so should this be a failure?

For the other cases, shouldn't we assert that we know the cases are bad, then when we fix the cases in followups these tests will start failing, and we will be forced to update them? As we are skipping deprecations this test will get out of date if we are simply ignoring the things it reports instead of making explicit assertions.

🇬🇧United Kingdom longwave UK

Committed 13ffc8b and pushed to 11.x. Thanks!

🇬🇧United Kingdom longwave UK

Thanks everyone for the discussion, agree that an improved comment is the best we can do here without a full rework.

Backported down to 10.5.x as a docs-only fix.

Committed and pushed 027156e5d1f to 11.x and 3e757eaca9c to 11.2.x and 1d1593ff63c to 10.6.x and 7abe563e1de to 10.5.x. Thanks!

🇬🇧United Kingdom longwave UK

Makes sense to me too, didn't realise you could do this with PHPStan but it will certainly help at higher levels.

Backported down to 10.5.x as a docs-only fix.

Committed and pushed be6b43929c7 to 11.x and 1f4bb78c9e8 to 11.2.x and 661fea386bd to 10.6.x and 14eba7b7a34 to 10.5.x. Thanks!

🇬🇧United Kingdom longwave UK

I don't have a problem with this, we are usually wary of using private anyway in Drupal because we are quite permissive about what we allow downstream users to do, and this seems to be no exception - especially as this is tests only, if someone really wants to override this behaviour in a test, what are we to stop them?

As a feature this is only eligible for minor releases, but I also see no harm in backporting this to 10.6.x.

Committed and pushed bb6ce4e4d46 to 11.x and 27ec7850b62 to 10.6.x. Thanks!

🇬🇧United Kingdom longwave UK

Thanks! Backported to 11.2.x as an eligible bug fix. This could also go back to 10.5.x and 10.6.x but given the use of OOP hooks in the test this will need reworking, marking as fixed for now but if someone wants to open a backport branch then please feel free.

Committed and pushed 75628d32c8a to 11.x and d163dd95b38 to 11.2.x. Thanks!

🇬🇧United Kingdom longwave UK

Published the change record node (but not the change record itself)

🇬🇧United Kingdom longwave UK

Overall I am +1 on doing this, and it will be a long road to getting it done everywhere in core, but added some questions to the MR.

🇬🇧United Kingdom longwave UK

Let's try to add some backward compatibility via moved_classes and/or class_alias().

🇬🇧United Kingdom longwave UK

Added a question to the MR.

🇬🇧United Kingdom longwave UK

Thanks for keeping up with this - let's land this now to avoid yet another rebase.

Committed 46c6a19 and pushed to 11.x. Thanks!

🇬🇧United Kingdom longwave UK

I get the same output in 10.6.x with

yarn
yarn vendor-update
yarn build:ckeditor5
yarn build:ckeditor5-types
🇬🇧United Kingdom longwave UK

longwave created an issue.

🇬🇧United Kingdom longwave UK
🇬🇧United Kingdom longwave UK

Copied all the classes that we use into core; DocParserTest still passes locally, let's see if anything else is broken. Unsure if we should copy any other tests from Doctrine.

🇬🇧United Kingdom longwave UK

We forked parts of Doctrine already by copying them into core namespaces, so I think the simplest solution is to do that for the remaining classes that we use.

🇬🇧United Kingdom longwave UK

Can we just reuse opcache.save_comments as the setting? If a site owner chooses to disable that, we skip processing annotations.

🇬🇧United Kingdom longwave UK

I suspect we can remove the jsdoc script and helper file entirely, will open a followup to discuss that.

I think I figured out why PHPStorm works with some files and not others; it's picking up the TypeScript definitions instead of the jsdoc, but CKEditor doesn't have @module tags in all the TS definitions.

command.d.ts has

/**
 * @module core/command
 */
...
export declare class Command extends /* #__PURE__ */ Command_base {

and this makes @extends module:core/command~Command work. But editor.d.ts doesn't have a @module tag so it doesn't get detected.

🇬🇧United Kingdom longwave UK

Rebased against 11.x, the performance test changes were unmergeable and will need redoing, otherwise the only big change was the conversion of system_page_top() to OOP.

🇬🇧United Kingdom longwave UK

Fixed up the jsdoc script to handle multiple exports per file. The second regex wasn't matching anything any more so I have removed that one.

🇬🇧United Kingdom longwave UK

Oops, didn't spot those jQuery UI changes, I remember this happening before but I don't remember why, the only change is that ,"ignoreList":[] is added/removed from the end of the map. Reverted this.

Re #14 I think the missing parts are a new bug in the jsdoc script. Previously it only looked for a single default export per file, now there can be multiple exports but I think we only pick up the first. Will see if I can improve this.

🇬🇧United Kingdom longwave UK
🇬🇧United Kingdom longwave UK

I think I fixed the jsdoc parser so it outputs the correct information for CKEditor5 v46 and above, still not sure if this is needed but I think it's as correct now as it was in the previous version.

I've also updated some of the existing jsdoc references for the renames that have occurred in CKEditor.

I think PHPStorm is now picking up the TypeScript types from CKEditor which is why I don't see much difference locally either way, but then e.g. this line still doesn't work locally and I don't understand why:

@extends module:core/plugin~Plugin

Still, that's probably out of scope here now given the jsdoc file appears to be largely correct again.

🇬🇧United Kingdom longwave UK

Doesn't look like this got pushed.

🇬🇧United Kingdom longwave UK

Firefox still doesn't have support for multiple import maps, but the end of the year is getting closer and we will need this for any CKEditor release in 2026 for 📌 Use New installation methods for CKEditor5 Active

Given that most other browsers do support multiple import maps I think we should go ahead and use the polyfill here.

🇬🇧United Kingdom longwave UK

The changes aren't too bad, just if you are importing code from the API you might need to make some minor renames as per the linked document. The much bigger change comes in 📌 Use New installation methods for CKEditor5 Active which we will have to adapt to soon, because they are stopping support of the DLL loading method at the end of the year.

Some of the jsdoc types need updating as per https://ckeditor.com/docs/ckeditor5/latest/updating/nim-migration/migrat... - this is quite tedious and perhaps can be deferred to a followup. As an example

@param {module:engine/model/element~Element} modelElement

becomes

@param {module:engine/model/element~ModelElement} modelElement

I'm still not clear about #8, because PHPStorm appears to discover some types whether or not the types whether ckeditor5.types.jsdoc exists or not.

🇬🇧United Kingdom longwave UK

Didn't mean to change status.

🇬🇧United Kingdom longwave UK

I don't really understand the jsdoc build step, but in PHPStorm I can get contextual documentation about any of the imports in the custom plugins without the jsdoc file anyway. I think the break is because of https://ckeditor.com/docs/ckeditor5/latest/updating/nim-migration/migrat... - but perhaps we just don't need this any more?

🇬🇧United Kingdom longwave UK
🇬🇧United Kingdom longwave UK

Actually DLL builds still work, we just need to make some plugin updates I think. A few exports have been changed and injectCssTransitionDisabler() is now CssTransitionDisablerMixin which I couldn't find an upgrade guide for, but seems to work.

🇬🇧United Kingdom longwave UK

The big problem here is that DLL builds are no longer supported and we have to migrate to using JS modules instead, so this is blocked on 📌 Use New installation methods for CKEditor5 Active

🇬🇧United Kingdom longwave UK

yarn build:ckeditor5-types deletes core/modules/ckeditor5/js/build/ckeditor5.types.jsdoc, with the following output:

[09:19:08] CKEditor 5 types have been generated: 0 declarations aliased, 1041 files ignored

Need to figure out what has broken here.

🇬🇧United Kingdom longwave UK

We didn't do this as quickly as I hoped, and v47 is now out; let's try to upgrade to that.

🇬🇧United Kingdom longwave UK

Committed and pushed e4a89e5ef80 to 10.6.x and f1f81bba162 to 10.5.x. Thanks!

🇬🇧United Kingdom longwave UK

We have EditorialContentEntityBase and the generic revision UI now, not sure what else we need to do here.

🇬🇧United Kingdom longwave UK

Things have moved on a lot, with the current work of moving the hooks to OOP and issues like 🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active I think this can be closed as outdated.

🇬🇧United Kingdom longwave UK

Only one person requested this, over eight years ago. This is a niche feature that I don't think we need to support in core given the lack of interest, therefore closing this one out.

🇬🇧United Kingdom longwave UK

Thanks for the swift turnaround, let's try this again.

Committed e2aea6e and pushed to 11.x. Thanks!

🇬🇧United Kingdom longwave UK

I am still in favour of doing this as per #39 and #45.

#11 is 5 years old, referencing a blog post from 8 years ago; things have moved on since then.

🇬🇧United Kingdom longwave UK

This is a nice cleanup. Doesn't backport cleanly to 10.x, not sure it is worth the additional effort.

Committed f7bf351 and pushed to 11.x. Thanks!

🇬🇧United Kingdom longwave UK

Gah, reverting.

🇬🇧United Kingdom longwave UK

Given this is a slight behaviour change as discussed in #17 through #26 I think this is best to commit to a minor only.

Committed and pushed 386cfa84bdb to 11.x. Thanks!

🇬🇧United Kingdom longwave UK

Looks good to me too. As it contains a database update this is only eligible for 11.x.

Committed b500857 and pushed to 11.x. Thanks!

🇬🇧United Kingdom longwave UK

Fixed up the docs, removed database drivers for now, and I wrote a basic change record at https://www.drupal.org/node/3549907

🇬🇧United Kingdom longwave UK

Yep the screenshot in #3 is saying you can't upgrade to symfony/console 7.3.0 because chi-teck/drupal-code-generator isn't compatible - this is a dependency of Drush, you can either remove Drush or try to upgrade it at the same time.

🇬🇧United Kingdom longwave UK

Composer is telling you that something else requires symfony/console 7.3.0. Try composer why-not symfony/console 7.3.0 which might give you additional clues as to what that is, or if you post your composer.json someone might be able to figure it out.

🇬🇧United Kingdom longwave UK

Fixed up a couple of comments and added types to ::theme().

🇬🇧United Kingdom longwave UK

Removed ThemeEngineInterface::getExtension(), the theme engine is directly responsible for appending file extensions now.

Otherwise, are we done here? Or is there more preprocess to deprecate somewhere?

🇬🇧United Kingdom longwave UK

I think this is just a duplicate of 🐛 RowPluginBase::render() update docblock. Needs work

🇬🇧United Kingdom longwave UK

Added credits.

🇬🇧United Kingdom longwave UK

Added credits.

🇬🇧United Kingdom longwave UK

longwave created an issue.

🇬🇧United Kingdom longwave UK

Tagging as a highlight, improving performance is always good.

🇬🇧United Kingdom longwave UK

This is getting super tricky to deprecate extension.list.theme_engine while still actually needing to use it for various tests, maybe have to revert this and leave it as an unused service that we can just quietly remove in Drupal 13.

🇬🇧United Kingdom longwave UK

Yeah it's not clear in many cases if we should be resetting one, some, or all. Not sure if we need to mess with any of that here though, if there are no known bugs; in practice the lists do not change often.

🇬🇧United Kingdom longwave UK

Following #81 the prefix property on theme value objects is used in a couple of tests, and more importantly ThemeSettingsForm, where we have another magic function name:

    if ($theme) {
      // Call engine-specific settings.
      $function = $themes[$theme]->prefix . '_engine_settings';
      if (function_exists($function)) {

I think we can just drop this with no replacement? I had no idea this existed, and if there are custom theme engines somewhere in the wild then they can alter this form in their new custom module instead now.

🇬🇧United Kingdom longwave UK

Unsure if or how to provide BC in the constructor here, but I also don't see why anyone would override this.

Production build 0.71.5 2024