Handle upgrading and rendering not yet upgraded component instances of components with newly removed props or slots

Just like at #3568602-2: Handle upgrading and rendering not yet upgraded component instances of components with new(ly) required props that include an example value: beautiful issue summary! Thanks, @effulgentsia! 💙

AFAICT the "removed prop" part of this issue is already being discussed/addressed as part of #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? 🤔

Do we want to reduce the scope of this issue to just "removed slots"?

A consequence of the above is that if someone removes a prop/slot from their component, then adds it back a short while later, component instances that haven't been edited in the meantime continue functioning as before (their old prop/slot values were never removed, so effectively get restored).

+1 for thorough test coverage for this. 🙏

Upon upgrading the component instance, remove child component instances from slots that don't exist in the component's active version.

😮🤔 This is the BIG one! This could quite literally undo/lose significant amounts of carefully crafted content.

Are we sure that this is the approach that we want? We could alternatively do something like (pseudocode):

if ($component_instance->getComponent()->loadedVersionIsActiveVersion()) {
  // no-op
  return;
}

$actually_existing_slot_definitions = $component_instance->getComponent()->loadActiveVersion()->getSlotDefinitions();
foreach ($component_instance->getComponentTreesBySlot() as $slot_name => $component_tree) {
  // Slot still exists
  if (array_key_exists($slot_name, $$actually_existing_slot_definitions)) {
    continue;
  }
  
  // Slot that disappeared.
  \assert($component_tree instanceof ComponentTreeItemList);

  // Empty: no data to save, early return.
  if ($component_tree->isEmpty()) {
    continue;
  }

  // "Rescue" this component tree:
  // 1. generate a new container component instance
  // 2. give it a visible label matching the prior slot name (retrieved from `fallback_metadata`)
  // 3. create it as a sibling of the current element
  // Bonus: make that container component be one of a new source — or perhaps the "Personalization" source: "only visible when viewed inside the Canvas UI".
  $lost_sheep = $this->createLostSheepContainerWithHeading(
    subtree: $component_tree,
    heading: $fallback_metadata[$slot_name]['label'],
  );
  $component_instance->getParent()->addSibling($lost_sheep);
}

👆 That would:

1. avoid the inevitable loss-upon-editing
2. instead empower the Content Author to decide on their own whether it'd be worth keeping the "lost sheep" or not
3. still avoid broken rendering, and never reveal the "lost sheep" to visitors of the site

I understand the concern about potential data loss, but I believe the simpler approach is the right one. Deleting the orphaned components aligns with established patterns. When you delete a folder, its contents are deleted. When you remove a field from a content type, the data is deleted. Or in a database schema, when you drop a column, the data is gone.

This is also how Drupal's existing page builders handle the same scenario. In Layout Builder, inline blocks that are removed from a layout due to a region being removed, become orphaned and get cleaned up asynchronously. When a paragraph field is removed from a content type, the paragraph entities may linger in the database temporarily, but get deleted later by garbage collection.

In both cases, the philosophy is: stop rendering removed content, then clean it up. Neither system attempts to "rescue" orphaned content by creating new visible containers. I'm concerned that creating sibling components would likely confuse content authors more than help them. When a developer removes a slot from a component, that's a deliberate decision. Because we only upgrade on edit, there's already a grace period. If someone removes a slot then realizes it was a mistake and adds it back, component instances that weren't edited retain their data. This provides a reasonable recovery window without adding complexity.

Alright, if it's intentional, all good! 👍 Thanks for confirming!

Gotta fix some tests

Chatted with @penyaskito and @effulgentsia. We're going to drop anything related to rendering, since it's safe to render the component with extraneous data (old prop inputs and old slots) until the component is upgraded.

We're removing this part of the solution:

When rendering a not-yet-upgraded component instance (component instance's version not equal to the component's active version), remove prop values and slot children from the ephemeral component instance data prior to rendering. In other words, when rendering, don't pass prop and slot values that aren't expected (aka supported!) by the component's code.

This item still remains valid

A consequence of the above is that if someone removes a prop/slot from their component, then adds it back a short while later, component instances that haven't been edited in the meantime continue functioning as before (their old prop/slot values were never removed, so effectively get restored).

Clean up the proposed solution a bit with our de-scope of handling anything with rendering. We can worry about that in #3568602: Handle upgrading and rendering not yet upgraded component instances of components with new(ly) required props that include an example value which has actual consequences when rendering (new required props.)

MR looks great overall! 👏

I have data loss concerns about the auto-update-upon-publishing-auto-saves that this MR introduces. See MR for details.

I think this is all set for the next review! Data loss issues reported in #13 should be mitigated, that approach was removed

Marking RTBC with penyaskito approval, but I want wim leers ++ based on data concerns in an earlier approach.

Leaving to `to be ported` in case there is follow ups to address, which I can handle in #3568602: Handle upgrading and rendering not yet upgraded component instances of components with new(ly) required props that include an example value

Reviewed retroactively per #15.

This looks amazing — thanks to the STR by @penyaskito, @mglaman nicely researched & elegantly solved the key challenge this MR surfaced. 👏

Needs follow-up for:

• one thing that was missed and isn't currently triggerable but will be: https://git.drupalcode.org/project/canvas/-/merge_requests/540#note_690261
• having @lauriii confirm whether that elegant fix didn't violate what @lauriii wrote in #3, because the resulting UX is slightly different

And just to be safe: @mglaman: can you confirm https://git.drupalcode.org/project/canvas/-/merge_requests/540#note_690262?

Now needs @lauriii to confirm the second bullet in #19.

Nope, per @penyaskito's response to the last link in #19, it looks like we're missing test coverage for an edge case. Can you provide STR, @penyaskito?

having @lauriii confirm whether that elegant fix didn't violate what @lauriii wrote in #3, because the resulting UX is slightly different

It would be preferable to not show anything as changed when applying updates because we're applying the same update during run time so from the perspective of an end-user, nothing has changed. I suggested that if we continue on the current path, we should at least show a toast to inform that the content was updated so that they know why it shows up as changed.

I'm afraid I cannot reproduce that as is. However found a different issue (testing with branch at #3568602: Handle upgrading and rendering not yet upgraded component instances of components with new(ly) required props that include an example value, but shouldn't be affecting this) which might be related. I've created #3574026: Component instances are not updating in the same way in regions than other entities to check this.

tl;dr: everywhere in Canvas we are previewing auto-save stuff, but not for page_regions, where we ignore auto-saved stuff and use published versions.

Should this have a change record?

#24: That's not correct. Updates don't happen on runtime. If your changes are done in a non-BC way, your published page is broken.

I think https://www.drupal.org/node/3571790 covers it? I just added this issue to that CR. Adding more detail to the CR is welcome but not essential IMHO.

Thanks for #3574026: Component instances are not updating in the same way in regions than other entities!

Cross-posted with #26.

I propose the two of you meet to get to the bottom of this 🙏

#27: We didn't specify which component changes are upgradable, so no change records needed then.

I propose the two of you meet to get to the bottom of this 🙏

The bottom of what? This had laurii feedback and approval on the proposed solution.

@mglaman, you wrote:

I figure this fixes the _technical_ requirement with what patterns we currently have, and then we can work with @lauriii to refine product experience.

— https://git.drupalcode.org/project/canvas/-/merge_requests/540#note_689199

I asked Lauri to confirm that. 😊

AFAICT he basically did confirm it's fine in #26 but requested a follow-up? (we should at least show a toast to inform that the content was updated so that they know why it shows up as changed. — tagging for that.)

But @penyaskito in #24 pushed back against @lauriii in #26. @penyaskito DM'd me about this: he's concerned about a mismatch in expectations. That is what I suggested the two of them to meet about.

Removed from scope, as rendering components with extraneous data does not break rendering. When the component is upgraded, data will be cleaned up.

That's true here.
Isn't true for #3568602: Handle upgrading and rendering not yet upgraded component instances of components with new(ly) required props that include an example value, which is what I misunderstood.
Apparently we still need some """"updates"""" (quotes intended) for rendering with new required props.

But for #24: They won't be the same updates. They would be just adding required prop values default values, not the entire update (e.g. deleting removed slots contents, deleting removed prop values)

re #24: created #3574253: Flag Canvas component trees that have been auto-updated on pending changes API call and #3574257: Provide a toast informing editors about which Canvas component trees have been auto-updated.

Thanks all!

AFAICT the bug that @mayur-sose reported at #3574743: 500 error for code component preview after deleting required prop from in-use Code Component is strongly connected to this issue.