🇳🇱Netherlands @Spokje

Probably should be in the release notes.

Nope, not improving my mood.

Besides a warning in the config validation which I don't understand:

$ git checkout -f $CI_MERGE_REQUEST_DIFF_BASE_SHA
fatal: reference is not a tree: 93b747d5e3f13dc80426d9891f34c94afe179278

all seems well.

Well, that was easier than expected...

Thanks, tests are now green, as expected.
Code changes make sense.

RTBC for me.

The nightwatch job failed, not sure if fluke or bad, and can't rerun since MR was created by a core committer.

Unconfused myself, it problem lies in the other file of the comparison: core/tests/PHPStan/composer.json.

Shouldn't that file be updated automagically when bumping PHPStan in the root composer.json?
Is this worth a follow-up?

Hmm, confused now:

1) Drupal\PHPStan\Tests\EnsurePHPStanVersionsMatchTest::testVersions
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'1.11.7'
+'1.11.8'
/builds/issue/drupal-3463954/core/tests/PHPStan/tests/EnsurePHPStanVersionsMatchTest.php:17

vs

https://git.drupalcode.org/project/drupal/-/merge_requests/8938/diffs#14...

Spokje → created an issue.

Tests are still green.

For the record, just removed the resolutions-section and did a $ yarn install.

I think we can now also drop the resloutions section from core/package.json.
These were only needed to keep the dependencies of the old version of nightwatch clashing with our other JS-dependencies.

bbrala → credited Spokje → .

The MR solved our use case (having drupal/memcache enabled, but neither Memcache nor Memcached installed), but is at best "iffy".

Not even sure how this situation came to be, since it seems to be not possible to enabled this module without having either Memcache or Memcached installed (See also #3136253: Disable memcache on local or dev environments (Drupal 8) → ), but here we are.

A "proper" solution would include decent tests, probably(?) a dedicated setting and the possibility to install without having either Memcache or Memcached installed.

But foremost, we need a blessing from the maintainer(s) if they want to support such a solution.

Putting this on NR in the hope this will attract that attention.

The same happened for me.

memcache_admin_post_update_add_service_definitions invalidated the container, as intended, and afterwards local/development/whatever environments with this module enabled whilst having neither Memcache nor Memcached installed fail with a 500 Uncaught PHP Exception Drupal\memcache\MemcacheException: "No Memcache extension found".

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

After watching a server die in the first test-run:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[08S01]:
Communication link failure: 1053 Server shutdown in progress:

, the second test came out just fine => RTBC for me.

Sadly this needs a reroll after the update of CKEditor landed

Lemme have a go at this, as I presume that @longwave is busy enough with more important RC1 stuff (like all other core committers ofc)

Code changes make sense and tests are green => RTBC

Fun stuff:

Apparently, if we don't add a core/.npmrc file with engine-strict=true, the restrictions in core/package.json "engines" are happily ignored.
https://stackoverflow.com/a/50798589

in which case why do we have glob 11 which only supports node 20?

Not sure but the whole MR was created using:

$ nvm use lts/hydrogen
Now using node v18.20.3 (npm v10.8.1)

Maybe glob isn't enforcing its own restrictions? *shrug*

- Tested MR!455 against the site on which I created the screenshot in the IS. Double check-boxes are gone, there's now only one, as expected.
- JS changes make sense.

This is RTBC for me.

Thanks for the _very_ quick turn-around!

Spokje → created an issue.

*sigh*

Well, that's all (I can do for this issue) folks!

Apples are green, but since we have a cspell-bump, let's also do a $ yarn spellcheck:make-dict.

With all of the above mentioned dependencies taken out, we're left with:

@floating-ui/dom ---^1.6.5  => ^1.6.7 
cspell -------------^8.8.1  => ^8.10.4
glob ---------------10.3.14 => 11.0.0 
jsdom --------------^24.0.0 => ^24.1.0
postcss-preset-env -^9.5.11 => ^9.6.0 
postcss ------------^8.4.38 => ^8.4.39
prettier -----------^3.2.5  => ^3.3.2 
terser -------------^5.31.0 => ^5.31.1
webpack ------------^5.91.0 => ^5.92.1

Let's see how Testbot likes them apples!

Regarding:
-

Bah, we can't update stylelint because of stylelint-formatter-gitlab: https://gitlab.com/leon0399/stylelint-formatter-gitlab/-/issues/3

@longwave in #3439522-16: Update JavaScript dependencies for Drupal 10.3/11.0 →

Seems like stylelint-formatter-gitlab is basically abandoned.

Since we shouldn't be held back by test-formatting-tools to upgrade our tooling, we might want to look for an alternative.
This might be one, although not a composer, but rather a JS dependency: https://github.com/tijsverkoyen/stylelint-formatter-gitlab-code-quality-....

Unsure how to proceed with this.

nightwatch breaks tests (won't say per usual, whoops I just did) when doing the major upgrade (https://git.drupalcode.org/project/drupal/-/merge_requests/8714) as well as the minor upgrade (https://git.drupalcode.org/project/drupal/-/merge_requests/8715).

Since I'm not exactly sure what the status is on this with regards to the (great) lullabot/* improvements lately, I need some guidance if we want to try and do this update for drupal 11 or not.

Not creating a child issue for that until that happens.

glob is doing a major jump from 10.3.14 to 11.0.0, which should give it a child issue, however, in a test-MR I prepared earlier, there are no test-failures when upping it.
(See https://git.drupalcode.org/project/drupal/-/merge_requests/8713)

I therefore think we can roll that upgrade into this issue.

Current situation:

$ yarn upgrade-interactive

? Pick the packages you want to upgrade.          Current          Range            Latest

   @ckeditor/ckeditor5-alignment --------------- ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-autoformat -------------- ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-basic-styles ------------ ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-block-quote ------------- ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-code-block -------------- ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-editor-classic ---------- ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-editor-decoupled -------- ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-essentials -------------- ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-heading ----------------- ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-horizontal-line --------- ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-html-support ------------ ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-image ------------------- ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-indent ------------------ ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-language ---------------- ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-link -------------------- ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-list -------------------- ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-paste-from-office ------- ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-remove-format ----------- ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-show-blocks ------------- ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-source-editing ---------- ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-special-characters ------ ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-style ------------------- ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @ckeditor/ckeditor5-table ------------------- ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   @floating-ui/dom ---------------------------- ◉ ^1.6.5 ------- ◯ ^1.6.7 -------
   ckeditor5 ----------------------------------- ◉ ~41.3.1 ------                  ◯ ~42.0.0 ------
   cspell -------------------------------------- ◉ ^8.8.1 ------- ◯ ^8.10.4 ------
   eslint -------------------------------------- ◉ ^8.57.0 ------                  ◯ ^9.6.0 -------
   glob ---------------------------------------- ◉ 10.3.14 ------ ◯ 10.4.4 ------- ◯ 11.0.0 -------
   jquery-ui ----------------------------------- ◉ ^1.14.0-beta.2                  ◯ ^1.13.3 ------
   jquery -------------------------------------- ◉ 4.0.0-beta ---                  ◯ 3.7.1 --------
   jsdom --------------------------------------- ◉ ^24.0.0 ------ ◯ ^24.1.0 ------
   nightwatch ---------------------------------- ◉ 2.4.2 -------- ◯ 2.6.25 ------- ◯ 3.6.3 --------
   postcss-preset-env -------------------------- ◉ ^9.5.11 ------ ◯ ^9.6.0 -------
   postcss ------------------------------------- ◉ ^8.4.38 ------ ◯ ^8.4.39 ------
   prettier ------------------------------------ ◉ ^3.2.5 ------- ◯ ^3.3.2 -------
   stylelint-config-standard ------------------- ◉ ^34.0.0 ------                  ◯ ^36.0.1 ------
   stylelint-prettier -------------------------- ◉ ^4.1.0 -------                  ◯ ^5.0.0 -------
   stylelint ----------------------------------- ◉ ^15.11.0 -----                  ◯ ^16.6.1 ------
   terser -------------------------------------- ◉ ^5.31.0 ------ ◯ ^5.31.1 ------
   webpack ------------------------------------- ◉ ^5.91.0 ------ ◯ ^5.92.1 ------

Spokje → created an issue.

Should we do something similar to 📌 Update Composer dependencies for 11.0.0-rc1 Needs review for JS-dependencies?

Fine by me, RTBC.

Ah ok, so my question boils down to: If we raise the recommended version and the version we run tests on is 1.11.7, should we still advertise the 1.11.1 version as a minimum, or should we bump to 1.11.7 in the root composer.json?

Highly unlikely this will break something in this instance, but might do in a future update and it "just" feels weird to allow a minimum version that we don't actively test on.

But n=1, yadada and if it's decide that's OK, I'm fine with it.

As far as I can tell the latest commit on the MR changes the version of PHPStan core/tests/PHPStan/composer.json to 1.11.7?
https://git.drupalcode.org/project/drupal/-/merge_requests/8701/diffs?co...

Dropped a comment in the MR.

Added a 10.3.x MR, which is basically the diff from the 11.x MR, without the deprecations.

Thanks sherlock mstrelan, seems like it's indeed not needed.
RTBC for me.

Thanks @benjifisher for the (very clear) rewrite of the IS and @berdir for explaining why the current solution is what it is, and might/could/should be the way forward.

Putting this up for review once more, so this major issue won't get stalled/lost-in-limbo.

I think you've beaten Moriarty @mstrelan!

Since this adds a PHPCS-rule and might break contrib/external-CI in some places, I think we need a CR for this.
Besides that, I'm all for RTBC.

De-confusing comment confirmed, RTBC for me.

Seeing that this was for rdf, would this be the place it was set: https://git.drupalcode.org/project/rdf/-/blob/2.x/rdf.module?ref_type=he...

If it is, it was removed from core with the move of rdf to contrib.

Nice, this even detected the sole usage in Olivero, so I'm more than happy to RTBC this.

I've decided not to create any follow-ups, since this shows we have a working solution for this case and I'm against Death-By-Admin, but if anyone wants to fill out templates and create a new issue, I'm all for it.

The deprecation process of theme-template-stuff is not very clear, (at least to me): https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... → .

Looks like the current deprecation message isn't added to the ['node']['variables'] return of node_theme(), but instead is overwriting it, leaving all node-theming logic without the actual node. Hence the gazillion test-failures.

If I look here, we can only deprecate the root level itself and not anything specific below it? https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

If I use that option for this issue, we're getting a deprecation warning _every_ time node_theme() is used.
We _could_ ignore that deprecation warning in core/.deprecation-ignore.txt, but maybe this would be the time to introduce deprecation at lower levels of theming functions return arrays?

- Updated deprecated/removed version numbers
- Added `@` to `trigger_error`, which, AFAICT, is still the default for `E_USER_DEPRECATED`.
- Added `E_USER_DEPRECATED` to `@trigger_error` in `\Drupal\user\Form\EntityPermissionsForm::access`

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

mikelutz → credited Spokje → .

mikelutz → credited Spokje → .

Spokje → changed the visibility of the branch 11.0.x to hidden.

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

Wonder if it's too late for 10.3-beta too?

The 10.x branch will stay on SF 6.4 AFAIK.

Since the parent has priority "Critical", bumping this issue to the prio as well.

(Look mam, I'm doing critical stuff!)

Spokje → created an issue.

@catch: Ah, of course, this eludes me every once in a while. Thanks for explaining.

Moving to correct version.

Thanks @catch.

Since there's not yet a version-tag for 11.1.x in the dropdown (nor Git), I think the correct status for this issue would be postponed and we whack a [11.1.x] in the title to have some kind of visual queue?

Idea is nice, code looks fine, tests are green.

The only thing that I wonder about if there's still time to deprecate/remove in 10.3.0/11.0.0 since the BETAs of both are already out.
I _think_ we're too late and need to deprecate/remove in 10.3.1/12.0.0, but am not sure
I know I've seen documentation on this before somewhere, but my search-fu is failing me now.

Putting this on RTBC so core committers can decide on this.

Spokje → changed the visibility of the branch 10.4.x to hidden.

Spokje → changed the visibility of the branch 3406971-non-silenced-deprecation-message to hidden.

Spokje → changed the visibility of the branch 11.x to hidden.

Spokje → changed the visibility of the branch 3406971-coder-rule to hidden.

$ composer diff
| Prod Packages                   | Operation | Base         | Target     |
|---------------------------------|-----------|--------------|------------|
| symfony/console                 | Upgraded  | v7.1.0-BETA1 | v7.1.0-RC1 |
| symfony/dependency-injection    | Upgraded  | v7.1.0-BETA1 | v7.1.0-RC1 |
| symfony/error-handler           | Upgraded  | v7.1.0-BETA1 | v7.1.0-RC1 |
| symfony/event-dispatcher        | Upgraded  | v7.1.0-BETA1 | v7.1.0-RC1 |
| symfony/filesystem              | Upgraded  | v7.1.0-BETA1 | v7.1.0-RC1 |
| symfony/finder                  | Upgraded  | v7.1.0-BETA1 | v7.1.0-RC1 |
| symfony/http-foundation         | Upgraded  | v7.1.0-BETA1 | v7.1.0-RC1 |
| symfony/http-kernel             | Upgraded  | v7.1.0-BETA1 | v7.1.0-RC1 |
| symfony/mailer                  | Upgraded  | v7.1.0-BETA1 | v7.1.0-RC1 |
| symfony/mime                    | Upgraded  | v7.1.0-BETA1 | v7.1.0-RC1 |
| symfony/process                 | Upgraded  | v7.1.0-BETA1 | v7.1.0-RC1 |
| symfony/psr-http-message-bridge | Upgraded  | v7.1.0-BETA1 | v7.1.0-RC1 |
| symfony/routing                 | Upgraded  | v7.1.0-BETA1 | v7.1.0-RC1 |
| symfony/serializer              | Upgraded  | v7.1.0-BETA1 | v7.1.0-RC1 |
| symfony/validator               | Upgraded  | v7.1.0-BETA1 | v7.1.0-RC1 |
| symfony/var-dumper              | Upgraded  | v7.1.0-BETA1 | v7.1.0-RC1 |
| symfony/yaml                    | Upgraded  | v7.1.0-BETA1 | v7.1.0-RC1 |

| Dev Packages         | Operation | Base         | Target     |
|----------------------|-----------|--------------|------------|
| symfony/browser-kit  | Upgraded  | v7.1.0-BETA1 | v7.1.0-RC1 |
| symfony/css-selector | Upgraded  | v7.1.0-BETA1 | v7.1.0-RC1 |
| symfony/dom-crawler  | Upgraded  | v7.1.0-BETA1 | v7.1.0-RC1 |
| symfony/lock         | Upgraded  | v7.1.0-BETA1 | v7.1.0-RC1 |

Spokje → created an issue.

GitLabBot found a few errors.

a handful of quick fixes in MR!7547 cut it down by about 25%.

Great, but not sure this is part of this issue? Or even what an acceptable amount of added errors to a baseline would be?

Personally, (n=1, yadayada) this seems like follow-up issue material to me and might be derailing the original intent?
But what do I know? _shrug_

3.9.2 landed and made it all better.

To be fair, we can still:

- Bump twig/twig to 3.9.1 which fixes Exception: Warning: Undefined variable $blocks.
- Bump to that version in composer.json, since lower version won't pass with the yield changes.

Did that just now.

longwave → credited Spokje → .

Ah, thanks

With 3.9.1 less errors, more Since twig/twig 3.9: Twig node "Foo" is not marked as ready for using "yield" instead of "echo"; please make it ready and then flag it with the #[YieldReady] attribute..

https://github.com/twigphp/Twig/issues/4008 Still messes with (at least) our Twig deprecation testing.

Not a big fan of lowering versions of dependencies, but seeing that:

- None of our other dependencies need it
- Tests are green
- PHPUnit 10 waits for no man...

I'm going to RTBC

Looks like we've bumped into https://github.com/twigphp/Twig/issues/4008

Spokje → created an issue.

Added two CRs.

Let's bump it to RTBC and see if the core committers think we still need FM approval and if so, if they can ping them :)

Thanks @quietone, good enough for me :)

- Approach makes sense, seeing there is precedence.
- Code changes make sense (Personally I find the new logged warning a lot more clear than before).
- Tests are green.

The only thing from RTBC-ing this is the CR link, which needs, at least for me, mentioning the new required LoggerInterface $logger, in Roles::__contruct().

One argument against staggering would be that raising the PHPStan level would be disruptive to (almost) all open MRs for each bump, so bumping more then one level would make it less disruptive.

That might be the case when the baseline was in the NEON format, and IIRC was one (of many) reasons the Bump-to-L9 issue is stalling.
But as part of that issue, we switched the baseline over to PHP with the reasoning that Git would be much smarter about merging it.

Is bumping a PHPStan level (with each one bringing in loads of changes on the baseline file) still as disruptive to other MRs as we (or at least I) think, or is this a non-issue by now?

If we're in non-issue land on that, I'm all about staggering, if not, we might want to prevent all/a lot of open MRs grinding to a rebase-halt each time we stagger-bump and do it in one go?

Thanks @mstrelan, should have found that myself, but somehow didn't. (Makes mental note to check caffeine-percentage of new bought coffee-brand).

Seeing those checks and also the fact that if we want to do it with the least amount of disruptions to other MRs, it's probably now or when D12 arrives, I'm pulling the trigger and put the ball in core commiters court: RTBC for me.

Looks like when we bump to PHPStan L2 we get an initial addition of ~7800 suppressions.
Seeing the amount of active issues to tackle the L1 suppressions, I think it's safe to assume these won't go away quickly.

So the benefit would be preventing new L2 issues creeping in new/refactored code.

It's probably my lack of Google skills, but what are the extra checks in L2 compared to L1, what will be prevented from creeping in by us doing PHPStan L++?

Ah crap, of course, forgot that in my "rage" about the baseline formatting.

📌 Bump phpstan/phpstan and mglaman/phpstan-drupal to latest Needs review probably needs to land to make this MR work.