🇧🇪Belgium @wim leers

I think this better conveys the intent?

@mglaman shared more detail in Slack. The way he runs into it is using a type: string, contentMediaType: text/html prop. That was out of scope in 🐛 Hero component failed to render after removing text Active (which was only about type: string aka plain strings). The relevant issue for that problem space is 📌 Invalid prop errors should be detailed in Component preview Active .

The client is not sending some any value for the required prop to the server. And so the server is correctly detecting that.

#6 is out of scope here. The scope of this issue is about using the information the client already receives (a JSON schema for every prop of an SDC or code component, including whether it is required or not) to improve the UX-while-typing in the component instance form

That's just because the class you changed it to doesn't have that method :) We just need an assertion that this is in fact still the expected implementation (Canvas' own).

🐛 References must not be required to guess props' JSON schema Active paved the path to REMOVE Canvas' class, because we assume ✨ [PP-1] Allow schema references in Single Directory Component prop schemas Postponed will land! 👍

However, since then …  #3556327: Don't save `Component` config entities unnecessarily → also ran into problems with decorators — see https://git.drupalcode.org/project/canvas/-/merge_requests/378#note_633849. So it stopped using decorators. It now does:

    // @todo Remove this once Canvas relies on a Drupal core version that includes https://www.drupal.org/i/3352063.
    $container->getDefinition('plugin.manager.sdc')
      ->setClass(ComponentPluginManager::class);
    // @todo Remove in clean-up follow-up; minimize non-essential changes.
    $container->setAlias(ComponentPluginManager::class, 'plugin.manager.sdc');

rather than

  Drupal\canvas\Plugin\ComponentPluginManager:
    decorates: plugin.manager.sdc
    parent: Drupal\Core\Theme\ComponentPluginManager

I forgot about https://git.drupalcode.org/project/ui_patterns/-/blob/2.0.x/src/Componen... — sorry! It's been a hell of a ride towards 1.0 🫠

So I think you're saying that Canvas MUST switch back to service decoration? And if so: how is this not a duplicate of 🐛 ComponentPluginManager decorator should call decorated service instead of parent Active ? If so: happy to push that forward 😊

#17: 👍

One of the failing tests is actually wrong, and cannot possibly be reproduced in Canvas itself. So, skipped it: https://git.drupalcode.org/project/canvas/-/merge_requests/358/diffs?com... — see #3532414-10: Follow-up for #3500386: tighten `::collapse()` to improve data integrity, because new props added to auto-saved code components cannot have a widget anyway → for why. @penyaskito confirmed this statement with manual testing.

@penyaskito manually tested this and confirmed #10.

🚢

\Drupal\Tests\canvas\Functional\ApiLayoutControllerTest::testWithDraftCodeComponent() is currently testing an impossibility.

The UI only can instantiate components based on Component config entities. Draft code components may make changes to props, but until they're non-draft, the Component config entity would never be updated.

So: it's impossible in the UI to trigger this scenario:

    // Add the code component into the layout.
    $uuid = 'ccf36def-3f87-4b7d-bc20-8f8594274818';
    $component_id = JsComponent::componentIdFromJavascriptComponentId((string) $code_component->id());
    $component = Component::load($component_id);
    assert($component instanceof Component);
    $json['layout'][0]['components'][] = [
      'nodeType' => 'component',
      'uuid' => $uuid,
      'type' => $component_id . '@' . $component->getActiveVersion(),
      'slots' => [],
    ];
    $props = [
      'name' => 'Hot stepper',
      'voice' => 'shouting',
    ];
    $json['model'][$uuid] = [
      'resolved' => $props,
      'source' => [
        'name' => [
          'sourceType' => 'static:field_item:string',
          'expression' => 'ℹ︎string␟value',
        ],
        'voice' => [
          'sourceType' => 'static:field_item:list_string',
          'expression' => 'ℹ︎list_string␟value',
          'sourceTypeSettings' => [
            'storage' => [
              'allowed_values_function' => 'canvas_load_allowed_values_for_component_prop',
            ],
          ],
        ],
      ],
    ];

This test is essentially testing a desired future state, where the code component developer would be able to instantiate draft code components that gained new props. i.e. it's trying to leap forward into a world where 🌱 Plan to allow editing props and slots for exposed code components Active is fully done.

I specifically made sure in #12 that this would NOT need me 😇

This blocks ✨ [PP-2] Only regenerate `Component`s whose `StorablePropShape` depends on invalidated cache tags Postponed . And it'd be a performance improvement when saving code components.

wim leers → created an issue.

#3556327: Don't save `Component` config entities unnecessarily → is in! That is a big step in the right direction. But much remains to be done.

Oh, and this should have updated relevant docs — this did not update docs/components.md. 😬 I propose to be pragmatic and tackle that in 🐛 Canvas' hook_storage_prop_shape_alter() must allow passing cache tags — to know when to re-evaluate Active .

If this is reproducible using Canvas, it must be possible to write a test for it.

For now, I'll settle for steps to reproduce 😇

LGTM.

To address #5.3, we could solve that using either PHPStan or PHPCS.

An LLM (Kimi K2) gave me these:

Three things I forgot:

1. I disagree with . It's literally called Canvas' Internal HTTP API. If and when we want to start a public HTTP API, I'd say it should have a different path prefix. Perhaps /canvas/api/public/v{n}?
2. I'd like to add
3. I worry that immediately after this effort of sprinkling @internal over hundreds of files, we'll regress. Because we're adding new (PHP) files all the time. How do we ensure that we won't regress? Wouldn't it be safer to require EVERY file to have either @internal or @api in its file-level docblock?

Thanks both @penyaskito and @penyaskito for finding the root cause for properly fixing things instead of reintroducing HEAD's work-around which I offered as an escape hatch in #21 to keep your sanity. You both certainly went above and beyond to get config schema checker exclusions to work in Cypress tests! 🤯 (And entertained me 🤣👏)

Thanks @phenaproxima for creating those follow-ups! I think 📌 Remove BlockComponent::componentIdFromBlockPluginId Active and 📌 Remove GenerateComponentConfigTrait Active make for nice novice issues, while 📌 Stop calling ComponentSourceInterface::checkRequirements() Active , 📌 Consider testing component status validation generically Active and ✨ Add the ability for ComponentSourceManager to generate an entity for a single specific component Active are above that "novice" bar.

ComponentSourceManager::generateComponentsForSource() brings so much more sanity to the way Components are created and updated, whereas before it required hours of debugging (which Christian and I did on Monday). It also made the update path tests have sane, explainable results (which is one of the reasons this issue was opened!). So this quite directly helps improve update path confidence.

See you all in 🐛 Canvas' hook_storage_prop_shape_alter() must allow passing cache tags — to know when to re-evaluate Active , which @penyaskito happily informed me was immediately green upon merging in 1.x that included this commit! 🥳

Refined proposal.

We already discussed #3 on Monday, and we already agreed on all that?

+1, because that would essentially be the same as what Drupal core does.

The difference with Canvas 1.x today is that the only public APIs are the ones declared in canvas.api.php. To achieve #3, we'll need to sprinkle hundreds of @internal over our codebase.

P.S.: API.md WFM too.

Validation added: https://git.drupalcode.org/project/canvas/-/merge_requests/358/diffs?com....

This means anybody should now be able to push this forward, because test failures are MUCH clearer now: helpful validation errors rather than PHP exceptions/fatal errors/500 responses.

Next steps: make tests pass, by doing:

• hopefully mostly: update tests to use the field type/static prop source specified in the Component config entity
• when that is not viable: update the Component config entity for which an instance is created to use the field type/static prop source that the test is actually using — which may be absolutely reasonable to do for testing DX 👍 I've provided one example, in CanvasPageVariantTest (which is one of the tests intentionally doing some quite funky stuff).

P.S.: I suspect that a few traits or test modules should help us get to green faster?

Actually, core did comply with 📌 Add generic interface + base class for upgrade paths that require config changes Active — the update path was just incomplete: #3274635-153: [upstream] Use CKEditor 5's native

and
UX → .

@phenaproxima: could you please review that core MR? @catch tells me that would need to land very soon.

Initial MR up. Still needs expanded update path tests.

Hm … @catch pointed me to https://git.drupalcode.org/project/drupal/-/commit/e1178387b0a87b9dda6bd... — I missed Ckeditor5Hooks::editorPresave(), sorry! 🙈

The update path should result in all Text Editor config entities using CKEditor 5 and its ckeditor5_list plugin getting the styles key in addition to reversed and startIndex.

The update path that was added here only does so if both the ckeditor5_list and ckeditor5_sourceEditing CKE5 plugins are active for a given Text Editor. That's an accurate way of determining whether styles should be TRUE or FALSE.

But if ONLY ckeditor5_list is present, styles should always be set to FALSE. That part is missing here.

#6 said "so I'd call this a major bug ", which is inaccurate.

No Drupal module in the world can provide shipped config that is considered valid by both 11.2 and 11.3 due to the way ✨ [upstream] Use CKEditor 5's native and UX Needs work landed in core.

@penyaskito explains why in #13: 👈 That's indeed the problem, and is a bug in core. So, reopened the core issue: ✨ [upstream] Use CKEditor 5's native and UX Needs work . It needs to follow 📌 Add generic interface + base class for upgrade paths that require config changes Active , which is what Canvas does, precisely to avoid this problem.

I'd prefer to see this fixed in core rather than worked around here. There's plenty more 11.3 failures in Canvas: 📌 Add Drupal 11.3 to the testing matrix Active .

Postponing this issue on 📌 Add Drupal 11.3 to the testing matrix Active on the Canvas side and the core update path on the core side.

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... → 🤔

This introduced a regression, due to the way its update path was implemented.

ckeditor5_post_update_list_type() cannot update shipped config. Which means this broke any module or recipe that uses this CKEditor 5 plugin and was targeting Drupal 11.2, because it does not provide an automatic update path to 11.3.

This should have followed the pattern described in 📌 Add generic interface + base class for upgrade paths that require config changes Active : it should be triggered in Editor::preSave().

First known module to break due to this: Canvas. See ✨ [upstream] Use CKEditor 5's native and UX Needs work .

📌 Fix schema issues in ckeditor config provided in shipped editors config Active is already fixing an 11.3-specific failure.

FYI: 🐛 Linking and unlinking a field does not correctly reset the form view to it's initial state Active landed: https://git.drupalcode.org/project/canvas/-/commit/67099cd866736cdd37854... on November 26.

Merged in 11.x, still the same exact CI results: 93 errors, 29 skipped.

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".)

wim leers → changed the visibility of the branch 3556327-improve-component-discovery--refactor-component-config-entity-saving to hidden.

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

A lot fails.

Thanks for creating this! Added some more relevant information.

Note that worst case, we could bring back what HEAD did to fix #20:

try {
  $component->save();
}
catch (SchemaIncompleteException $exception) {
  if (!str_starts_with($exception->getMessage(), 'Schema errors for canvas.component.sdc.sdc_test_all_props.all-props with the following errors:')) {
    throw $exception;
  }
}

All of this because \Drupal\TestSite\Commands\TestSiteInstallCommand is incomplete :|

The Cypress e2e test failure in prop-types.cy.js is legitimate.

[versioned_properties.active.settings.prop_field_definitions.test_string_format_date.default_value.0] 'value' is an unknown key because versioned_properties.active.settings.prop_field_definitions.test_string_format_date.field_type is datetime (see config schema type field.value.datetime).

triggered by installing sdc_test_all_props: https://www.drupal.org/files/issues/2025-12-03/Screenshot%202025-12-03%2... →

Reviewed all MR comments.

In doing, I now do have a naming opinion 😅 @larowlan's naming suggestion kinda clicked for me: it results in 4 quite clear phases in the life cycle! I think @phenaproxima indeed aptly questioned the name too, but I think his proposed alternative name is actually more confusing than what's in HEAD from another perspective.

Also, RE: naming. I don’t really care. This is all internal for now anyway. It will change anyway.

I’ll let penyaskito pick — I made choices, Adam and Lee proposed alternatives, he picks 🤓😄

this is a major last-minute refactor and therefore is of course a little messier than we'd ideally like.

It is last-minute because nowhere near enough time was allocated to address known technical debt: that associated with “let’s build a working PoC and then iterate”.

That approach is GOOD! It is what enables this MR to barely write new code, but identify patterns across diverse use cases.

Together with the vast test coverage, it’s feasible to do such a significant refactor with confidence 😊

The only bad bit is that it’s indeed so last-minute. 😞 (Our hand is forced because the MR that resurfaced this would have made Canvas overall much more brittle. This MR helps prevent that.)

The key insight I see here is that "component discovery" is its own thing now, no longer tightly coupled to plugin discovery (which of course doesn't hold up for certain kinds of components, most notably JS components). That decision by itself makes an enormous amount of sense.

This!

And crucially, again: the same complex bits of logic, just combined more coherently.

Obviously plugin discovery still triggers component discovery, but it's not the only thing that can do it.
Actually, it doesn’t. Only saving code components triggers it. Other than that, only hook_rebuild() triggers it.

And the issue this will unblock then will “simply” introduce a new trigger: the cache tags associated with the hook_storage_prop_shape_alter() implementations — because a new Media Type may be relevant to populate for example video or image props.

Pushed a commit to #3556327: Don't save `Component` config entities unnecessarily → 's https://git.drupalcode.org/project/canvas/-/merge_requests/378 that should be reverted which outlines how this IMHO paves a nice path for this MR: https://git.drupalcode.org/project/canvas/-/merge_requests/378/diffs?com...

1. 2 failures remain — hoping @penyaskito can tackle those: https://git.drupalcode.org/project/canvas/-/pipelines/677242/test_report...
2. Pushed a commit that should be reverted which outlines how this IMHO paves a nice path for 🐛 Canvas' hook_storage_prop_shape_alter() must allow passing cache tags — to know when to re-evaluate Active : https://git.drupalcode.org/project/canvas/-/merge_requests/378/diffs?com...

Should be down to 2 failures now. Assume virtually zero availability from me tomorrow, please get this across the finish line

@mohit_aghera: that now has a near-RTBC MR: #3556327: Don't save `Component` config entities unnecessarily → .

Check if we're in an update kernel inside ComponentPluginManager and BlockManager before saving or updating component config entities.

Done:

🐛 Text formatted with CKEditor within Canvas gets double escaped when output Needs work - where we only got 2 versions of an SDC component in an update hook vs 3 for a JS component

👆 This is fixed by this MR, see https://git.drupalcode.org/project/canvas/-/merge_requests/378/diffs?com...

… but it also surfaced a bug in the update path that 🐛 Deleting or renaming SDC prop will break xb pages using old version of the component Active added: the Components it generated are actually invalid (trivially proven, I wish we thought of it back then: https://git.drupalcode.org/project/canvas/-/merge_requests/378/diffs?com...), so thanks to this MR correctly updating and saving Components, it is actually causing ComponentTrackingRequiredPropsUpdateTest to fail, because it didn't expect new Component versions to be created. And now there are new versions appearing. Why is that?

Because after updates finish running (i.e. after the UpdateKernel!):
, all caches are flushed, which triggers hook_rebuild(). That is what is creating new versions.

In other words: this is now auto-detecting that the update path test fixture is wrong! Worked around it in https://git.drupalcode.org/project/canvas/-/merge_requests/378/diffs?com..., needs follow-up.

While working on #3556327: Don't save `Component` config entities unnecessarily → 's !378 MR, I stumbled upon \Drupal\Tests\canvas\Kernel\Config\ComponentTest::testComponentAutoUpdate(), which is literally about what this issue is describing:

  /**
   * @see media_library_storage_prop_shape_alter()
   * @see \Drupal\Tests\canvas\Kernel\MediaLibraryHookStoragePropAlterTest
   */
  public function testComponentAutoUpdate(): void {

This MR should update that test.

And now all of BlockComponentTest is passing.

Now running entire test suite, then will start pushing SDC/code component stuff 👍

The problems this MR are running to that I've alluded to in #32 and #33, but now called out explicitly:

• the saving of Components during discovery, even if nothing has changed, is the root cause for why @penyaskito was debugging these tests for several days
• working around it is seemingly simple, but causes other failures
• those other failures may in theory be fixable, but the logic is so incredibly spread out that doing that is very risky: we're dealing with interactions here of:
  1. component source plugin
  2. associated decorated plugin manager or config entity storage, which triggers Component::save()s
  3. 👆 those 2 are already wildly inconsistent in how they interact with each other, and which is responsible for what AND each component source plugin implementation + associated decorator does things differently
  4. a cache collector's updateCache() method calling upon destruction (i.e. during KernelEvents::TERMINATE)
  5. a mind-boggling amount of cyclomatic complexity

⇒ this re-surfaces/stresses the need for making "component sources" a solid, supported, public PHP API. But that is out of scope for 1.0. Yet we want to tag 1.0, and have 1.0 be maintainable. So even though we should do #3556327: Don't save `Component` config entities unnecessarily → and really all of 🌱 [META] Production-ready ComponentSource plugins Active , we can't.

⇒ tackling the foundational parts in #3556327, see #3556327-9: Don't save `Component` config entities unnecessarily → . I'm already well on my way: https://git.drupalcode.org/project/canvas/-/merge_requests/378. @penyaskito is already reviewing. 👍

Progress: BlockComponentTest::testDiscovery() is passing!

AFAICT this MUST be fixed to fix 🐛 Canvas' hook_storage_prop_shape_alter() must allow passing cache tags — to know when to re-evaluate Active — see #32 + #33 on that issue.

Nice, thanks!

#7++

#6 is inaccurate.

It’s absurd to drop 11.2 compatibility at the last second just to be compatible with an unreleased upcoming core version.

We need to be compatible with both. And not sure how, yet. This feels like a bug in core for not auto-updating Editor config entities?

There are no issues; 11.3 changed config (schema).

That linked commit alone is not enough to make the tests pass.

Fixing it is a slippery slope towards more complexity. Because a bit of foundation is missing.

Working to add that. To be continued tomorrow.

The changes to src/Plugin/Canvas/ComponentSource/JsComponent.php that https://git.drupalcode.org/project/canvas/-/merge_requests/353 introduced caused the PHP linting CI job to fail: https://git.drupalcode.org/project/canvas/-/jobs/7504625

RE: #10 + #11 + … + #18 and the issue that @larowlan created for it: #3556327: Don't save component config entities during discovery in an update kernel →

Those unconditional Component::save() calls in the (SDC and JS) component sources are causing cache tag invalidations after Dynamic Page Cache has been populated … which is the root cause of what @penyaskito has been debugging for days: https://git.drupalcode.org/project/canvas/-/merge_requests/145/diffs?com...

IOW: +1 for fixing #3556327: Don't save component config entities during discovery in an update kernel → , and actually this issue requires some of that to be fixed (i.e. the commit above). Specifically, we should only perform actual saves of Components when something actually changed. That's what I described at #3556327-6: Don't save component config entities during discovery in an update kernel → , and which the linked commit does.

Given the amazing upstream contributing experience, I asked the jsonrainbow/json-schema maintainer whether they'd be interested in accepting a PR from us to add recursive $ref resolving. That'd mean we could omit ComponentPluginManager::resolveJsonSchemaReferences() from this MR. Or, at least have to not maintain it forever. (It really does make sense to live in that project, after all.)

See https://github.com/jsonrainbow/json-schema/pull/853#issuecomment-3595541100

Oops, one tiny oversight: we should now be able to remove

    // @todo Remove this line once https://www.drupal.org/project/drupal/issues/3352063#comment-16363119 is fixed, or justinrainbow/json-schema's UriValidator has been fixed upstream.
    unset($refSchema['id']);

… to avoid continuing to carry this technical debt.

Can you update the issue summary and create those follow-ups, @penyaskito? 🙏

#27 makes sense, but is fairly open-ended. I think/hope you've had conversations with @larowlan that enables you to better update the IS+create followups than me?

I clarified the follow-up for the first bullet in #27: #3526045-13: Settle on final name for `ComponentSource` plugins (`ComponentType`?) + expand their scope (discovery, maybe more?) → .

Thanks!

This mammoth effort was indeed awesome to see land, but it didn't distinguish between SDCs and code components — it treated both equally.

That's how this ended up slowing down 🐛 References must not be required to guess props' JSON schema Active , because there a type: string, format: uri, enum: … test-only SDC was introduced. And the inevitable periods in URLs then were incorrectly deemed invalid.

So 🐛 References must not be required to guess props' JSON schema Active ended up refining what this did: it applied the "dot restriction" only to code components.

This came up again in ✨ Enable contrib to register `pattern` and `$ref` shape matches for prop shapes Active . When we do this, one more @todo pointing here will have to be updated.

Visual bug reported: 🐛 Prop description display is broken for boolean type Active .

This is a bug that slipped under the radar in 💬 SDC prop "description" is not used as the form element description? Active .

This should be looked at by the people who worked on 📌 Create an Image SDC that can be included by other SDCs Active , which introduced the canvas:image SDC.

I think this ought to be a stable blocker. Because it'll require bumping our versions.

Also, this was discovered independently by somebody else on Canvas: 🐛 Stream wrapper URIs are incorrectly flagged as invalid (hostname misinterpreted) Active — what a coincidence!

Wow: #3352063-69: Allow schema references in Single Directory Component prop schemas → → we can now just make Canvas require https://github.com/jsonrainbow/json-schema/releases/tag/6.6.2 to fix this!

Wow!!!

Can we now bump core's requirement to require 6.6.2?

I independently discovered this bug yesterday:

1. That led me to a whole saga of discoveries. I presumed justinrainbow/json-schema to correctly validate URIs. So I concluded that we'd need to change how json-schema-definitions:// worked. So I implemented that … and then discovered that actually, their URI validation is wrong:

— #3515074-32: Shape matching MUST work with the resolved equivalents of $refs AND must be compatible with core's upcoming $ref resolving in SDCs →

But the second one is a problem: our current shape matching infrastructure (see \Drupal\experience_builder\JsonSchemaInterpreter\SdcPropJsonSchemaType::computeStorablePropShape()) does not support adding matches (field types+widgets) for additional shapes.

Actually, this has not been true since forever? We've had \Drupal\canvas\Hook\ShapeMatchingHooks::datetimeRangeStoragePropShapeAlter() (or prior to OOPification: datetime_range_storage_prop_shape_alter()) since 📌 Clarify the "shape matching" bits: namespaces, `CODEOWNERS` and as issue queue component Fixed .

So AFAICT this statement by me in the issue summary is wrong:

🛑 The blocker: hook_storage_prop_shape_alter() only can alter the existing hard-coded shape matches in SdcPropJsonSchemaType::computeStorablePropShape(), it cannot add new ones.

This wasn't really a bug from a Canvas technical POV: this was a well-known @todo, known literally since the day we added $ref resolving in August 2024 (in 📌 Introduce an example set of representative SDC components; transition from "component list" to "component tree" Fixed ), where we added:

  /**
   * @param JsonSchema $schema
   * @return JsonSchema
   *
   * @see \Drupal\experience_builder\Plugin\Adapter\AdapterBase::resolveSchemaReferences
   */
  private static function resolveSchemaReferences(array $schema): array {
    if (isset($schema['$ref'])) {
      // Perform the same schema resolving as `justinrainbow/json-schema`.
      // @todo Delete this method, actually use `justinrainbow/json-schema`.
      $schema = json_decode(file_get_contents($schema['$ref']) ?: '{}', TRUE);
    }

    // Recurse.
    if ($schema['type'] === 'object') {
      $schema['properties'] = array_map([__CLASS__, 'resolveSchemaReferences'], $schema['properties'] ?? []);
    }
    elseif ($schema['type'] === 'array') {
      $schema['items'] = array_map([__CLASS__, 'resolveSchemaReferences'], $schema['items'] ?? []);
    }

    return $schema;
  }

I'm surprised it got us this far!

Funny enough, I wrote

Plus, we'll be able to delete that hook implementation once ✨ [PP-1] Allow schema references in Single Directory Component prop schemas Postponed is in.

on August 13, 2024, at #3446722-66: Introduce an example set of representative SDC components; transition from "component list" to "component tree" → , and … it still isn't in 😅 But this shows the intent has been to rely on making core do the right thing, since the very start!

Thanks everyone, and especially @pdureau: for articulating in #9 why this is important to get right before Canvas 1.0.

Change record created: https://www.drupal.org/node/3560491 → .

Docs updated.

Update path added. And justified in detail, with explicit +1 from @penyaskito:

Updated issue summary to explain that this issue covered 3 distinct problem areas. Not by choice, but by necessity.

Did all that.

In doing so:

1. The canvas_test_sdc:component-mismatch-meta-enum test SDC is now automatically being discovered as a Canvas-compatible SDC. This requires some test expectation updates.
2. The "dots in meta:enum" test coverage is ComponentValidationTest::testUnmatchedEnumAndMetaEnum(). It expected to find validation errors. Now it doesn't anymore. But the same exact component props schema should trigger validation errors if it is used for a code component, so did that: automatic conversion to that triggers the same validation errors, and actually one more: a fundamental config schema error that literally demonstrates why we had to do this in the first place 👍
3. Last but not least: a small @todo that #3516602 should've fixed had been forgotten, which buys us even more confidence in these changes, because it results in another "SDC meta:enum used as-is in a code component", but this one triggers no errors (because no dots in keys) 👍

Since @penyaskito is the expert in this area, assigning this for review to him. Only today's commits are relevant. Attached them as a perhaps easier-to-review patch.

wim leers → created an issue.

Thanks!

@penyaskito investigated the last remaining failure given his work on meta:enum that is causing the last test failures.

i.e. 🐛 Don't raise exception when an enum value is missing from meta:enum Active introduced meta:enum but then the follow-up introduced some magic: https://git.drupalcode.org/project/drupal/-/merge_requests/12326/diffs#4... — that magic is causing this the underscores to be converted back to periods.

20:25
Proposed resolution: relax our requirement for SDC, but we need to keep it for JS Components.
20:34
the bad: this validation needs to move somewhere else only affecting js components
the good: this would need to happen anyway because of e.g. canvas:cli
the ugly: we need to keep a similar logic still for js components, which means a) sdc and js differ; b) we need to keep both "." and "_" for the meta:enum to work with js components.

— @penyaskito

On it.

1. So, did what I said in my prior comment → led to Drupal\Core\Render\Component\Exception\InvalidComponentException: [props.properties.logo.id] Invalid URL format">
2. That led me to a whole saga of discoveries. I presumed justinrainbow/json-schema to correctly validate URIs. So I concluded that we'd need to change how json-schema-definitions:// worked. So I implemented that … and then discovered that actually, their URI validation is wrong:
   • #3352063-63: Allow schema references in Single Directory Component prop schemas →
   • #3352063-64: Allow schema references in Single Directory Component prop schemas →
   • #3352063-65: Allow schema references in Single Directory Component prop schemas →

   So I reverted it, and instead made SDC's $ref resolving simply not set the id post-resolving. Core will have to do the same until the upstream bug is fixed. 👍

3. Then started using the forward-ported-from core resolver (i.e. from #3352063 — this MR was already adding that!) in PropShape.
4. That revealed a bug we'd missed so far: this issue's goal was not yet met for array shapes. This was revealed thanks to relying on the SDC plugin manager providing fully resolved prop schemas, which will be the case after ✨ [PP-1] Allow schema references in Single Directory Component prop schemas Postponed lands.
5. That finally allowed removing $ref resolving from JsonSchemaFieldInstanceMatcher and AdapterBase. 🥳
6. Then: just tying up loose ends, nothing complex left.

All of that together ended up taking a net of 11 files changed, 107 insertions(+), 134 deletions(-) — aka net less code. 🥳

Calling it a day now. This was hell. 👹

I am in favor of questioning which JSON schema version we are targeting in Drupal Core.

+1

Should we move this issue to Drupal core instead?

Posted a one-line work-around to not have to do the very big amount of work that #65 describes: https://git.drupalcode.org/project/drupal/-/merge_requests/13457/diffs#n... — brought to you by my sibling Canvas MR at https://git.drupalcode.org/project/canvas/-/merge_requests/255.

Ship it!

As of https://git.drupalcode.org/project/canvas/-/merge_requests/255/diffs?com..., the MR was green and ready-to-be-merged: @phenaproxima's feedback had been addressed!

The changes are trivial: 10 LoC changed in /src, everything else is in /tests to be able to test all this against themes 👍

But … #3352063 is coming to core, and Canvas 1.0 must be ready

#30 reminded me that we really should address all the @todos that pointing here added by 🐛 Don't break other JSON Schema references Needs work . Because if we don't, we could paint Canvas into a corner once ✨ [PP-1] Allow schema references in Single Directory Component prop schemas Postponed lands in core. So, been working on that…

This will need an upstream bugfix, but we can work around it here.

This will still need that underscore test coverage I requested, because without it, module://content_translation/whatever would cause \JsonSchema\Tool\Validator\UriValidator::isValid() to fail.

Hm, checking https://www.ietf.org/rfc/rfc3986.txt:

   URI           = scheme ":" hier-part [ "?" query ] [ "#" fragment ]
…
   hier-part     = "//" authority path-abempty
…
   authority     = [ userinfo "@" ] host [ ":" port ]
…
   host          = IP-literal / IPv4address / reg-name
…
   reg-name      = *( unreserved / pct-encoded / sub-delims )
…
   unreserved    = ALPHA / DIGIT / "-" / "." / "_" / "~"

There it is: underscores are allowed.

So

([a-z0-9.-]+|\[[a-f0-9:.]+\])            # Hostname or IPv6 in brackets

in \JsonSchema\Tool\Validator\UriValidator::isValid() is wrong. Despite it literally referencing RFC 3986.

😑

Awesome to see this in! 👏

While it's not out in a stable release yet, can we double-check something? 🙏

Quoting myself from #3352063-63: Allow schema references in Single Directory Component prop schemas → :

Per https://git.drupalcode.org/project/drupal/-/merge_requests/13457#note_63..., I think ✨ Add stream wrappers to access extension files Needs work would need to be changed to actually generate valid URIs that JSON Schema would not consider validation errors.

Either that, or justinrainbow/json-schema's \JsonSchema\Tool\Validator\UriValidator::isValid() is wrong 😑

However … most other URI validators are fine with it. So perhaps … it is related to the fact that DNS host names aren't allowed to contain underscores, but non-DNS-based hostnames are fine? It is https://www.rfc-editor.org/rfc/rfc952 that dictates underscores are forbidden, but the same doesn't necessarily apply here.

Per https://git.drupalcode.org/project/drupal/-/merge_requests/13457#note_63..., I think ✨ Add stream wrappers to access extension files Needs work would need to be changed to actually generate valid URIs that JSON Schema would not consider validation errors.

Either that, or justinrainbow/json-schema's \JsonSchema\Tool\Validator\UriValidator::isValid() is wrong 😑

That's almost reassuring. But indeed, it matters which JSON schema version SDC targets and whether justinrainbow/json-schema resolves things correctly.

Which version of JSON schema are we targeting in Drupal 11.3?

https://git.drupalcode.org/project/drupal/-/raw/HEAD/core/assets/schemas... points to http://json-schema.org/draft-04/schema# — for which I cannot seem to find as nice a link as the one you provided (https://datatracker.ietf.org/doc/html/draft-pbryan-zyp-json-ref-03#secti...).

Added the test theme SDC to ensure that themes are equivalently supported, per @phenaproxima's #27.

Which led me to re-discover JSON Schema insanity: 🐛 SDC DX: Canvas' `/schema.json` declares `title` alongside `$ref` many times, but this is ignored by the spec, yet Canvas relies on it! Active .

Almost there.