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

👏 This is a great first step!

It avoids ALL complex cases. Best of all: all the necessary metadata is already present in Component config entities' per-version fallback_metadata.slot_definitions and settings.prop_field_definitions.

But the latter … only exists for SDCs and code components (and any other component sourced from a ComponentSource that uses GeneratedFieldExplicitInputUxComponentSourceBase). → Needs title+issue summary update

Citing type: canvas.generated_field_explicit_input_ux

canvas.generated_field_explicit_input_ux:
…
    prop_field_definitions:
      sequence:
        mapping:
          required:
…
          default_value:

Pseudo code:

  private function updatingToActiveVersionRequiresNoInputChanges(ComponentInterface $component): bool {
    \assert(!$component->isLoadedVersionActiveVersion());
    $original_version = $component->getLoadedVersion();

    $original_slots = $component->getSlotDefinitions();
    \assert($component->getComponentSource() instanceof GeneratedFieldExplicitInputUxComponentSourceBase);
    $original_props = $component->getSettings()['prop_field_definitions'];

    $component->loadVersion($component->getActiveVersion());
    $active_slots = $component->getSlotDefinitions();
    $active_props = $component->getSettings()['prop_field_definitions'];

    // @todo if any of the bullets listed in the IS are not met, return FALSE
    
    // Avoid side effects: restore originally loaded version.
    $component->loadVersion($original_version);

    return TRUE;
  }

Once that exists, calling something like this on every component instance in a component tree when loading it for editing in the Canvas UI would be sufficient:

  public function updateComponentInstanceVersion(ComponentTreeItem $component_instance): ?bool {
    $component_config_entity = $component_instance->getComponent();
    if ($component_config_entity->isLoadedVersionActiveVersion()) {
      // Nothing to do.
      return NULL;
    }

    // Try to auto-update.
    if ($this->updatingToActiveVersionRequiresNoInputChanges($component_config_entity)) {
      // Modify by reference.
      $component_instance->set(
        'component_version',
        $component_config_entity->getActiveVersion(),
      );
      // Auto-update of component version occurred!
      return TRUE;
    }

    // Auto-update of component version not possible.
    return FALSE;
  }

(That one is not pseudo code, this should actually work with zero changes.)

I had a similar approach in mind. Preventing what's to come later, and hopefully not over-engineering, I had planned:

1. In the component source definition, having a "updater: ComponentInstanceUpdaterInterface" as we do with the discovery.
2. That would provide isUpdateNeeded(ComponentTreeItem $component_instance), canUpdate(ComponentTreeItem $component_instance) and update(ComponentTreeItem $component_instance)
3. For Js and SDC, implement a GeneratedFieldExplicitInputUxComponentInstanceUpdater with what you had in mind.
4. For blocks, we would add it once #3521221: Block plugins need to be able to update their settings in multiple different storage implementations is resolved.
5. For the next issues in the parent meta, we would add an updateInteractively(ComponentTreeItem $component_instance), for those cases which can't be automatically updated (e.g. should a new required prop have the default value? or the user needs to select it?)
6. Then the update can be called when the component tree goes from autosave → publish (e.g. in preSave(), as with the existing upgrade paths).
7. To be defined (not to be covered here): user has to manually press a button for running updateInteractively, or we can try a PoC doing that when an existing component tree is → autosaved. In both cases the editor will be able to review, as the preview will be updated.

Thoughts?

I had a similar approach in mind.

🥳

1. 🟢 Oh, even better! That elegantly side-steps the limitations I pointed out! → Removing the issue tags you made obsolete 🤠
2. 🟡 What if a component instance can be updated to a later version but NOT the latest (aka active) version? Should happen eventually. In scope, or out of scope here, that doesn't matter to me.
3. 🟢 → that'd basically be the snippets in #3
4. 🟢 → YES!!!! 👏
5. 🟠 I … don't know what this method would do? How could a pure server-side bit of logic generate an interactive UX? I'd propose to entirely descope this. I think this means that for point 1, I think updater should be renamed to auto_updater or transparent_updater or something like that. That'd leave room for a future interactive_updater handler. WDYT?
6. 🟠 Agreed that should happen for all component instances that reference a non-active version of a Component config entity. But when a Canvas user starts editing a component instance, I'd expect this to run before the component instance form is populated. Because then the user immediately gets the latest UX, instead of only the next time they edit this entity.
7. 🙏 Agreed, but let's descope interactive updates entirely?

2. 🟡 What if a component instance can be updated to a later version but NOT the latest (aka active) version?

I thought having $from and $to arguments. But then realized: what's the point? I want to have the latest (active) version. What's the point if I can't? The only benefit I can think of is if we at some point "clean up" unused versions from Component config entities, as it will reduce the number of used versions. Is it worth given the complexities it adds?

🟠 I … don't know what this method would do? How could a pure server-side bit of logic generate an interactive UX? I'd propose to entirely descope this. I think this means that for point 1, I think updater should be renamed to auto_updater or transparent_updater or something like that. That'd leave room for a future The upgrade can happen automatically e.g. if handler. WDYT?

Maybe I picked a bad name. Let's say I add a required prop with a default value. I can upgrade automatically, but I'd still want the editor to review the outcome of this upgrade is what they expect. That's not in scope here, but I thought of this because I initially was thinking about naming this auto_updater too. But don't think we want to have 2 different handlers in the future when/if we support that. That's why I would go with a more generic updater

🟠 Agreed that should happen for all component instances that reference a non-active version of a Component config entity. But when a Canvas user starts editing a component instance, I'd expect this to run before the component instance form is populated. Because then the user immediately gets the latest UX, instead of only the next time they edit this entity.

Makes sense. Probably _in addition_ to what I said (to cover if I already edited something, it's in autosave, then I click publish AFTER the component has changed again but still allowing a non-interactive upgrade).

Discussed with @attilatilman and I’m taking over.

I want to have the latest (active) version. What's the point if I can't?

That we'd minimize the number of Component config entity versions being used by the site's component instances.

Benefits:

1. consistent UX (because different versions might use different widgets!)
2. and indeed what you wrote: The only benefit I can think of is if we at some point "clean up" unused versions from Component config entities, as it will reduce the number of used versions. Is it worth given the complexities it adds?

I'm fine with punting it to a follow-up. This alone is a huge leap forward. But then we should document the rationale for NOT doing this yet in the (code) docs. 🙏

Let's say I add a required prop with a default value. I can upgrade automatically, but I'd still want the editor to review the outcome […]

Aha!

So: this would NOT be applied automatically during ::getComponentTree(); it'd only be applied while loading the component instance's form? 🤔

Detailed review: https://git.drupalcode.org/project/canvas/-/merge_requests/485#note_665972

So: this would NOT be applied automatically during ::getComponentTree(); it'd only be applied while loading the component instance's form? 🤔

That was my first attempt, but:

• the component instance form only know about its version: not all the existing ones;
• even when changing that, it updated the form as expected, but not the actual preview (because their versions differ now!).

So doing it in :getComponentTree(); worked. We need to find a less aggressive way of doing that. Checking if normalizeForClientSide is viable is the next try.

Even better. ::getClientSideRepresentation. See https://git.drupalcode.org/project/canvas/-/merge_requests/485/diffs?com...

And https://git.drupalcode.org/project/canvas/-/jobs/8323270 failures indicate what I would have expected: we don't need lots of new test coverage, but ensuring the existing JsComponentEvolutionTest takes this into account (and at least is partially affected)!

Ready for reviews (manual testing also encouraged)

🏓 https://git.drupalcode.org/project/canvas/-/merge_requests/485#note_669327

Created #3571362: [PP-1] When editing a Canvas entity with a component tree (e.g., page, template, or region), automatically upgrade component instance versions to the most recent possible version where there's no impact on existing data

Created #3571366: Allow Canvas Patterns+PageRegions+ContentTemplates provided by Recipes to NOT specify component versions

Covered everything but

reuse existing infra rather than duplicating (long-term risk): !485 (comment 669326)

I don't see a clean way forward. Maybe tomorrow.
Assigning in the meantime for the rest.

Review

GeneratedFieldExplicitInputUxComponentInstanceUpdaterTest is 🤩👏 (and it's >50% of the MR!)

What I did on the MR

1. RE: #19 and a clean way forward: I iterated on this, because you rightfully pointed out that the DRY refactor I requested caused a performance hit. I walk you through what I did and why at https://git.drupalcode.org/project/canvas/-/merge_requests/485#note_672888
2. … and in doing so, the OG use case for #3571366: Allow Canvas Patterns+PageRegions+ContentTemplates provided by Recipes to NOT specify component versions now is already supported and tested, and I'm not convinced yet there's that many comparable use cases. I proposed a different direction for that issue — hope you like it!
3. Renamed the new interface and explained why: https://git.drupalcode.org/project/canvas/-/merge_requests/485#note_670476

Next up

1. Just one remaining concern: https://git.drupalcode.org/project/canvas/-/merge_requests/485/diffs#not...
2. @penyaskito to review what I did, and hopefully he is convinced 😇

MR already approved — it's now in the hands of @penyaskito. 😊 I don't feel the need to chime in further.

Zooming out: why did I push back?

I focused on data integrity/tech debt/long-term thinking.

The 3 chief concerns in my review(s):

1. long-term thinking: getting closer to a public Component Source API so the new "updater" handler needed to be consistent with the "discovery" handler we added ~1.5 month ago
2. technical debt: not spreading the "same shape as" or "compatible field data/definition" related logic out of low-level Canvas places into even more places, but instead reducing repetition — we already exceeded the rule of 3 previously, but this made MR pushed it far beyond, so I felt it important to centralize it
3. data integrity+long-term thinking: ensuring the path is paved for "old component instance allowed to use 1 image media type" can auto-update to a later version where 3 image media types are allowed

I thought I had missed 2 test cases:

1. deleting a subset of allowed target bundles (e.g. image + baby photos initially, delete one of the two)
2. deleting all allowed target bundles (which would cause Canvas to fall back to the image field type instead of an entity reference)

But neither is possible/allowed thanks to config dependencies: a Component relying on a media type (in any version of the Component config entity!) has a config dependency preventing such in-use media types from being deleted.

(The ability to safely delete unused media types at a later time would be a task/feature request for Canvas on its own — it'll require refining ::onDependencyRemoval() to check if all of the affected component versions have zero instances referring to it.)

Manually tested: works great. Tested:

• most common scenario: add new optional prop to SDC, drush cr, edit component instance ⇒ new prop appears!
• most complex scenario: create new image media type ⇒ the new image media type does automatically (and instantaneously → #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 works!) appear! 🎉

In doing so, I discovered one caveat for usability though: already auto-saved component trees do not get this automatic update. The automatic update works only when starting to edit a component tree for "the first time", not when continuing from a prior auto-save.

It is likely a user will get confused by this at some point. It is explainable, and it is possible to recover from, so I think it's fine as-is 👍🚢

GIF showing the instantaneous appearing of new image media types:

Yay 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 in December!

In doing so, I discovered one caveat for usability though: already auto-saved component trees do not get this automatic update. The automatic update works only when starting to edit a component tree for "the first time", not when continuing from a prior auto-save.

Actually, this worked fine, and it still does. Mea culpa!

I think this merits a change record? We should inform Canvas users (especially the advanced ones) that this frustration is now largely a thing of the past. It can include the GIF because it's the easiest-to-grok (because 100% UI-driven) change.

Or perhaps you want to do one over-arching change record for #3463996: [META] When the field type, storage/instance settings, widget, expression or requiredness for an SDC/code component prop changes, the Content Creator must be able to upgrade?

I will just say one word !
Youhou !

Draft change record: https://www.drupal.org/node/3571790

🥰🎉🚢🎂🤩🚀