Test Canvas on Drupal 11.3

what, if anything, fails, and try to fix that before 11.3.0 is released.

A lot fails.

Either daily or on-demand is sufficient.

In the MR, it's currently manually.

(Daily CI job is possible, but not in this MR: it's a GitLab CI project setting: "scheduled pipelines".)

#3560831: Update Canvas' CKEditor 5 config to allow using the native <ol type> vs <ul type> picker is already fixing an 11.3-specific failure.

Seems like there's also many label_display block plugin setting related failures. I don't see a CR for that: https://www.drupal.org/list-changes/drupal/published?keywords_descriptio... 🤔

Added #3562100: Remove usages of code deprecated in Symfony 7.4.

Looks like https://www.drupal.org/project/gitlab_templates just bumped the default from 11.2.8 to 11.3.1.

Which means Canvas' CI is now failing, HARD. Every in-progress MR is impacted.

(One hour ago, https://git.drupalcode.org/project/canvas/-/merge_requests/433/pipelines was passing, now it's failing across the board!)

EDIT: per https://git.drupalcode.org/project/gitlab_templates/-/commit/b935cd1f71d..., see #3563698: Drupal 11.3 and 10.6 are released - update variant variables, which shipped in https://git.drupalcode.org/project/gitlab_templates/-/commits/1.13.0?ref... ~1 hour ago 😬

⚠️ Note that we MUST continue to test 11.2. Canvas 1.x has 11.2 as the minimum, so it must continue to be tested.

It's probably best to change Canvas' CI to continue to inherit upstream changes (even though they broke here today, obviously). For now, the work-around I recommend is using https://project.pages.drupalcode.org/gitlab_templates/info/templates-ver..., like so: https://git.drupalcode.org/project/canvas/-/merge_requests/433/diffs?com...

Approved !449. It's a band-aid, but it gets us unblocked to achieve the actual objective of this issue.

Gonna try to push this forward per Wim's request.

Nice progress here, @phenaproxima!

Fascinating that #3547808: label_display block configuration value should not be translatable is so far responsible for the bulk of the changes.

Adding a related issue in core which broke some assertions of specific exception messages (these really should be using assertStringContainsString() for future proofing).

https://www.drupal.org/node/3539877 is the reason why some kernel tests now need new dependencies.

AFAICT #3571992: CI: Add "deprecations (php)" job that fails if deprecation count exceeds some threshold, to force chasing upstream PHP deprecations (and generate novice issues!) is impossible to solve until this is done, see #3571992-8: CI: Add "deprecations (php)" job that fails if deprecation count exceeds some threshold, to force chasing upstream PHP deprecations (and generate novice issues!). So much pain, induced by Symfony 🫠

#20: AFAICT #3563216: Add a base class for kernel tests to improve DX: `CanvasKernelTestBase` would help make achieving that a lot simpler, because then this MR has to update fewer files?

I am stuck on the last failing test, which is the very tricky \Drupal\Tests\canvas\Kernel\Plugin\Canvas\ComponentSource\ComponentInputsEvolutionTest.

I decided to take it one test case at a time, so I started with testBlockPluginUpdateConsequences().

Running this test on 11.3, the first failing assertions are these:

    self::assertSame('7cc894b85e93a7d8', $before->getActiveVersion());
    self::assertSame(['7cc894b85e93a7d8'], $before->getVersions());

And they are easy to fix. The hash is hard-coded for 11.2, but it's different in 11.3. Refactoring to this gets us moving again:

    $before_version = $before->getActiveVersion();
    self::assertSame([$before_version], $before->getVersions());

Onward! The next failing assertions are:

    self::assertSame('88c370526c14d185', $after->getActiveVersion());
    self::assertSame(['88c370526c14d185', '7cc894b85e93a7d8'], $after->getVersions());

And changing that to this, gets us going again:

    $after_version = $after->getActiveVersion();
    self::assertNotSame($before_version, $after_version);
    self::assertSame([$after_version, $before_version], $after->getVersions());

Chugging along here.

But then I hit a wall. I can't get past this assertion this assertion (sorry for the length):

    self::assertSame([
      'active_version' => 'The version 7cc894b85e93a7d8 does not match the hash of the settings for this version, expected ec03b64ff4f992b9.',
      'versioned_properties.active.settings.default_settings' => "'change' is a required key because source_local_id is canvas_test_block_input_schema_change_poc (see config schema type block.settings.canvas_test_block_input_schema_change_poc).",
      'versioned_properties.active.settings.default_settings.foo' => [
        'The value you selected is not a valid choice.',
        'This value should be of the correct primitive type.',
      ],
    ], self::violationsToArray($before->getTypedData()->validate()));

Specifically, that active_version violation, which fails thusly (massaged a bit for clarity):

Expected:
Array &0 [
    'active_version' => 'The version 7cc894b85e93a7d8 does not match the hash of the settings for this version, expected ec03b64ff4f992b9.',
]

Actual:
Array &0 [
    'active_version' => 'The version 0b5af0d270d99618 does not match the hash of the settings for this version, expected ecbfb3dfb7ce5717.',
]

The hashes have changed. That's expected, of course, but hear me out...

The first hash mismatch is straightforward; I just have to replace 7cc894b85e93a7d8 with $before_version.

But that second hash, ec03b64ff4f992b9 -- where the hell is that coming from? I can't figure it out! It's not the before version, it's not the after version, and if I switch the codebase back to 11.2, it's still not the before or after versions. I can't even generate this hash if I duplicate the logic that does the validation (\Drupal\canvas\Entity\Component::validateActiveVersion()) -- regardless of whether I'm on 11.2 or 11.3.

It's opaque to me. If can't figure out how this hash is generated, I have no hope of figuring out how to update the assertion properly.

Can anyone give me some guidance?

re: #23:

    // We need this test to pass both in 11.2.x and 11.3.x and above. Component versions hashes are influenced by their
    // config schema, and for blocks that means depending on the block.settings.*. As block_settings.label_display
    // changed between 11.2 and 11.3, that means there is no single block where we can have the same hash on 11.2.x and
    // above. So we need to hardcode these per version.

Ended up hard-coding versions for 11.2 and 11.3, and using match so a potential change again for 11.4 is accommodated if we end up supporting 3 core versions.

So we have 2 different hashes, before and after, and those depend on the config schema. I think that's what Adam was missing.

#3567413: When editing a Canvas entity with a component tree (e.g., page, template, or region), automatically upgrade component instance versions for cases where there's no impact on existing data didn't cover blocks, because that's blocked on core's #3521221: Block plugins need to be able to update their settings in multiple different storage implementations. But when config schema changes for blocks are released, there won't be a plugin update even after that (e.g. a new constraint alters the hash, but doesn't change the underlying block data, so no upgrade will exist). Wondering if we should cover that, and HOW 🤯? As is, component instances won't be able to auto-upgrade in that case. That's definitely out of scope here anyway.

After a painful merge, this is up-to-date with HEAD as of now.
This will be conflicting all the time, so let's try to merge this ASAP.

11.3 test failures are not blocking as is. I'd suggest we run them on every MR, or find a different way to ensure we add #[RunTestsInSeparateProcesses]. If not, I foresee this will be constantly missing and the test-run will never be green.

AFAICT ~80% of the pain here is caused by

-            'label_display' => FALSE,
+            'label_display' => '0',

but that's exactly what https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... fixed back in June for Canvas' #3528159: Ensure deterministic version hashes for ComponentSource-specific settings, thanks to config schema-powered normalization, and which core will eventually fix in #2544708: The `label_display` setting in `type: block_settings` has the wrong config schema type: then it will actually change to booleans, as you'd expect.

I'm surprised that in so many places we got away with not being strictly compliant with that. Did we figure out yet what change in 11.3 caused the need for all this to change?

Discussed with @penyaskito, and there is one upstream change that is causing the component versions to change, even if we had not gotten #27 wrong in Canvas:

diff --git a/core/config/schema/core.data_types.schema.yml b/core/config/schema/core.data_types.schema.yml
index 8bbffd2acb2..d7388c08af0 100644
--- a/core/config/schema/core.data_types.schema.yml
+++ b/core/config/schema/core.data_types.schema.yml
@@ -445,8 +456,14 @@ block_settings:
       type: label
       label: 'Description'
     label_display:
-      type: label
+      type: string
       label: 'Display title'
+      constraints:
+        Choice:
+          choices:
+            - '0'
+            - visible
+        NotBlank: []

→ this causes \Drupal\canvas\ComponentSource\ComponentSourceBase::generateVersionHash() to generate a different version hash, even if label_display had been set to '0', because:

    $normalized_data = [
…
      'schema' => $this->getExplicitInputDefinitions(),
    ];

👆 That uses the component source plugin-specific schema definitions — for SDCs that's JSON schema per prop, for block plugins that's the block plugin's settings' config schema.

Having investigated #27 + #28, I think this points to 2 larger systemic issues in Canvas:

1. Absence of validation when constructing test cases: it doesn't make sense to test Canvas infrastructure on invalid component trees: validation would prevent those from being created. This is the case in e.g. \Drupal\Tests\canvas\Kernel\Plugin\Canvas\ComponentSource\BlockComponentTest::testGetExplicitInput(), CanvasTestSetup, etc.
2. The block ComponentSource plugin goes through great pains to hide label and label_display from Canvas users, but then 1) actually goes back and restores it just prior to saving, 2) doesn't actually validate these UNLESS there's other block plugin settings.

   It also means pointless data ends up being stored:

   INSERT INTO `canvas_page__components` (`bundle`, `deleted`, `entity_id`, `revision_id`, `langcode`, `delta`, `components_parent_uuid`, `components_slot`, `components_uuid`, `components_component_id`, `components_component_version`, `components_inputs`, `components_label`)
   VALUES
   	('canvas_page', 0, 3, 6, 'en', 0, NULL, NULL, 'fc5bb8e6-7aa5-409f-bbda-37dcfa620b15', 'block.system_branding_block', 'b168140fdcaebae5', '{"use_site_logo":false,"use_site_name":true,"use_site_slogan":false,"label":"Site branding","label_display":"0"}', NULL);
   

   ⇒ Note the "label":"Site branding","label_display":"0" bit in there, that is the default block label (the block plugin label!) and the default value for label_display). Why even store that at all, for every single block component instance, if we don't allow configuring it?! Why not just omit it altogether and restore it just-in-time?

→ Needs follow-up to address both. But we shouldn't block this issue on it.

#29.1 → #3572847: Add missing validation to component trees being saved into content entities in tests— and simplify `CanvasTestSetup`

#29.2 → #3572850: "block" ComponentSource plugin should never store inputs for `label` and `label_display`, because they're not available for Canvas users

Thanks for the epic work on this, @phenaproxima! 🙏💙👏 And thanks @penyaskito for that final bit, and the nudge to prioritize this over everything else.

Review

1. 150 files, +764, −230 with most of those files being changed being trivial and forced upon us by upstream (PHPUnit)
2. 100% of the /src changes are to comply with upstream Symfony changes — CR: https://www.drupal.org/node/3554746. That will make this MR help #3571992: CI: Add "deprecations (php)" job that fails if deprecation count exceeds some threshold, to force chasing upstream PHP deprecations (and generate novice issues!) a lot — and that in turn should ensure we won't quite need repeats of this epic endeavour!
3. I reviewed 100% of the test changes to make sure that upon merging this we don't A) lose test coverage (we did, fixed that) nor B) cause more cursing at Canvas' test suite (this MR would've, fixed that).
4. As @penyaskito said: this should always be tested, not only manually, because then we'll instantly regress. I dropped the phpunit (>11.2-only features) CI job and instead made the phpunit (11.3) CI job run always, similar to the existing phpunit CI job (which doesn't say but tests 11.2 aka Canvas' minimum core requirement).
5. Follow-ups created for #30, and they both have initial MRs for others to continue. 🚀

⇒ Auto-merge queued:

chore: #3561392 Test Canvas on Drupal 11.3

By: phenaproxima
By: penyaskito
By: wim leers

🚢

Hah, just one day later, and an upstream change already caused a failing 11.3 CI job:

    Media Libraries Build (Drupal\Tests\canvas\Kernel\MediaLibrariesBuild)
     ✘ Library build
       ┐
       ├ Failed asserting that two arrays are equal.
       ┊ ---·Expected
       ┊ +++·Actual
       ┊ @@ @@
       ┊      0 => './core/themes/claro/css/base/...ts.css'
       ┊      1 => './core/themes/claro/css/base/...hy.css'
       ┊      2 => './core/themes/claro/css/base/print.css'
       ┊ +····3·=>·'./core/themes/claro/css/base/...es.css'
       ┊  )
       │
       │ /builds/project/canvas/tests/src/Kernel/MediaLibrariesBuildTest.php:104

— https://git.drupalcode.org/project/canvas/-/jobs/8470490#L2479

Why? Because this is targeting 11.3.x-dev, rather than a concrete tag:

…
Locking drupal/core (11.3.x-dev ff7efdf)
…

Let's target a specific 11.3 release to avoid this.

Likely cause: #3554220: Claro's libraries don't enforce the variables.css dependency's https://git.drupalcode.org/project/drupal/-/commit/a2ac2dc82289864c7cc29...

#3573473: Fix HEAD fixed #35, but @isholgueras' MR is doing the last bit of #35 — thanks!