Don't save `Component` config entities unnecessarily

FYI post update hooks still run with the update kernel, I don't know why they're treated differently honestly. Dove into this a while back: https://mglaman.dev/blog/hookupdaten-or-hookpostupdatename

💯 Confirming this problem:

(While debugging #3550334: [update path] [upstream] Text formatted with CKEditor within Canvas gets double escaped when output.)

Current issue scope

What this is proposing is changing \Drupal\canvas\Plugin\ComponentPluginManager::setCachedDefinitions():

  protected function setCachedDefinitions($definitions): array {
    parent::setCachedDefinitions($definitions);

    // Do not auto-create/update Canvas configuration when syncing config/deploying.
    // @todo Introduce a "Canvas development mode" similar to Twig's: https://www.drupal.org/node/3359728
    // @phpstan-ignore-next-line
    if (\Drupal::isConfigSyncing()) {
      return $definitions;
    }

… to also do an early return if inside the update kernel. Right?

Counterproposal

I think that actually the bigger problem is this other bit in that same method:

  protected function setCachedDefinitions($definitions): array {
…
    foreach ($definitions as $machine_name => $plugin_definition) {
…
      $component_id = SingleDirectoryComponent::convertMachineNameToId($machine_name);
      if (array_key_exists($component_id, $components)) {
        $component_plugin = $this->createInstance($machine_name);
        $component = SingleDirectoryComponent::updateConfigEntity($component_plugin);
…
      }
      else {
…
          $component = SingleDirectoryComponent::createConfigEntity($component_plugin);
…
      }
      try {
        $component->save();
      }
      catch (SchemaIncompleteException $exception) {
…

IOW: the blind Component::save() call there — even if NOTHING has changed about the SDC-on-disk! Right now any time the SDC discovery cache is cleared, we re-save its Component config entity, for no reason at all. If you look at \Drupal\canvas\Plugin\Canvas\ComponentSource\SingleDirectoryComponent::updateConfigEntity(), you'll see we're simply overwriting it, blindly:

    $component
      // These 3 can change over time:
      // - label and category (unversioned)
      // - settings (versioned)
      ->set('label', $definition['name'] ?? $component_plugin->getPluginId())
      ->set('category', $definition['category'])
      ->createVersion($version)
      ->deleteVersionIfExists(ComponentInterface::FALLBACK_VERSION)
      ->setSettings($settings);
    return $component;

and thanks to this bit in ::createVersion(), it avoids creating the same version again and again:

  public function createVersion(string $version): static {
    // Don't actually create a new version if the current version is the active
    // version. This improves DX: as long as the version (a deterministic hash)
    // remains the same, calling this won't have any effect (idempotency).
    if ($version === $this->active_version) {
      return $this;
    }

Conclusion: stop calling Component::save() when nothing has changed.

#3550334: [update path] [upstream] Text formatted with CKEditor within Canvas gets double escaped when output landed so this shouldn't be postponed anymore.

AFAICT this MUST be fixed to fix #3547579: Introduce a new cache tags aware prop shape repository, so changes affecting prop shape calculation can force the re-invoke of hook_canvas_storable_shape_prop_alter — see #32 + #33 on that issue.

And to do that, we must AFAICT do what I wrote 6 months ago at #3526045-7: Settle on final name for `ComponentSource` plugins (`ComponentType`?) + expand their scope (discovery, maybe more?)… 😅

The good news is: this is very doable, and should just require moving existing code around 😊

Progress: BlockComponentTest::testDiscovery() is passing!

And now all of BlockComponentTest is passing.

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

#3550334: [update path] [upstream] Text formatted with CKEditor within Canvas gets double escaped when output - 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 #3532514: Gracefully handle components in active development: ensure great DX 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.

EDIT: work-around reverted, because it caused widespred failures. We better keep the sanity this MR brings, and just fix the update path test fixture in a separate issue 🙏

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

Done: https://git.drupalcode.org/project/canvas/-/merge_requests/378/diffs?com...

EDIT: this somehow made ComponentWithRichTextShouldUseProcessedUpdateTest fail?! 🤯 I'll let @penyaskito (or somebody else) investigate that 👍

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

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 #3547579: Introduce a new cache tags aware prop shape repository, so changes affecting prop shape calculation can force the re-invoke of hook_canvas_storable_shape_prop_alter: https://git.drupalcode.org/project/canvas/-/merge_requests/378/diffs?com...

Okay, I just reviewed this.

In general, I see and like what you're laying down here. This is creating an actual API for discovering components, and that API feels like it has a solid core, even though this is a major last-minute refactor and therefore is of course a little messier than we'd ideally like.

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. Obviously plugin discovery still triggers component discovery, but it's not the only thing that can do it.

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.

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 🤓😄

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.

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

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 :|

Hoooookay, so today was @penyaskito's day to wrestle with the dragon. He traced it far enough to determine that it was a problem with the test site install command. But it was still utterly fscked, and Cypress was not helping by being so opaque. Christian also could not reproduce the problem locally.

I was luckier -- I could reproduce it locally. A few exceptionally puzzling hours of debugging led me to find out that we cannot be changing site-specific services.yml files without also invalidating the file cache that, er, caches them. This is an obscure detail in an obscure method of FunctionalTestSetupTrait. Can you believe that sh_t?

Anyway, the person who paid for this with his sanity was absolutely @penyaskito, who gave it everything he had on the eve of a stable release, and we all owe him a round of drinks. Even though the guy owns a wine bar.

I'm thinking this will pass tests now (notwithstanding the usual flakiness).

Beyond that, I have reviewed this MR thoroughly and I get what we're doing. It is a clear few steps down the path to the promised nirvana of sanity and maintainability. I think all follow-ups are filed. I don't think we need to delay this. Approved and RTBC, although I'll let someone else (@larowlan?) make the commit.

Okay, I lied.

I didn't do the bulk of the work on this issue, but I reviewed all open MR threads and they seem to be resolved, or in some cases deferred, satisfactorily. Lee and Christian have been over this thoroughly, as have I, and this is such a significant improvement that I don't think we should wait - let's get it in now and take advantage of it.

Merged into 1.x.

Thanks both @penyaskito and @phenaproxima 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 #3561267: Remove BlockComponent::componentIdFromBlockPluginId and #3561270: Remove GenerateComponentConfigTrait make for nice novice issues, while #3561265: Remove ComponentSourceInterface::checkRequirements(), #3561271: Consider testing component status validation generically, in `ComponentSourceTestBase` and #3561272: Add the ability for ComponentSourceManager to generate a Component config entity for a single source, or even a single specific component 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 #3547579: Introduce a new cache tags aware prop shape repository, so changes affecting prop shape calculation can force the re-invoke of hook_canvas_storable_shape_prop_alter, which @penyaskito happily informed me was immediately green upon merging in 1.x that included this commit! 🥳

Oh, and this should have updated relevant docs — this did not update docs/components.md. 😬 I propose to be pragmatic and tackle that in #3547579: Introduce a new cache tags aware prop shape repository, so changes affecting prop shape calculation can force the re-invoke of hook_canvas_storable_shape_prop_alter.