[PP-1] Autosave data should store converted server-side representation of model

This would be a massive change in the auto-save architecture.

Unfortunately, the ADR for that still hasn't landed: 📌 Document the reasons for not using Workspaces for saving in 0.2.0 Active . 😬

@tedbow, thoughts? :D

The only other way I can see us 'recovering' when a fallback component is stored in auto save AND the component it replaced reappears is if we have an event listener that listens to config events and detects the change of source plugin in a config entity from fallback to something else. The listener would have to loop over autosave data and pass the component props to the new source plugin and ask it to transform input data to client data and then re-save the entry. Now that I type that out, it might be less effort and more perfomant than making autosave do to/from client data on every read and write

I think it's worth exploring if we can detect that scenario with existing config events

Yes! this worked, updating issue summary and opening an MR, snuck in just under my 1hr timebox I allowed for it.

Wow, very cool to see that:

• this uses the infrastructure that 📌 Add a route for PATCHing both a config entity and its auto-saved version together Active added
• this basically does what I suggested over at #3515629-16: FieldWidget's XB transforms must be bubbled by the Field Widget rendering to inform the client → .2, where I sketched how I think this could actually happen automatically → .

Very cool!

With all of the other changes that have landed in the last few weeks, is this issue still relevant? If so, what are the steps to create a broken auto-save state?

Tagging as a beta blocker to answer this question and evaluate. Depending on the answer, we might decide that actually fixing it doesn't necessarily have to block a beta.

The steps to reproduce would be:

* Create a new menu
* Place an XB version of that menu's block in an unsaved page
* Delete the menu
* Try to render the component preview

But having said that, with the introduction of component versions, it might not be an issue.

I'll do some digging.

Rebased this off 0.x

We do still need it because when $component->getComponentSource() is using the fallback as the last version, the shape of the input data server side is different to that on for the default generated components.

Updated the logic to allow for the new versioned properties and got the test passing again.

wim leers → credited tedbow → .

Great call, @effulgentsia!

+1 to @larowlan's analysis in #11 + #12.

Test-only CI job fails 👍}

Clarifying the issue scope per #4 and what's in the MR.

I'm wondering if \Drupal\Tests\experience_builder\Kernel\Plugin\ExperienceBuilder\ComponentSource\FallbackInputTest::testFallbackInputCanBeRecovered() is not essentially obsolete thanks to \Drupal\Tests\experience_builder\Kernel\Plugin\ExperienceBuilder\ComponentSource\ComponentSourceTestBase::testFallback() since recent changes wrt when the Fallback plugin becomes active ( 📌 Version component prop definitions for SDC and Code components Active , 🐛 Component::onDependencyRemoval() should be an uninstall validator Active etc.).

In that ::testFallback() test coverage I also spotted

    if (static::class === BlockComponentTest::class) {
      // @todo Update Component entities with BlockComponent source plugin: https://drupal.org/i/3484682
      $this->markTestIncomplete('Block components do not yet update component config entities');
    }

… but 📌 Handle update and delete of Block component config entities Active landed a while ago. We forgot this bit. We can remove it with zero changes AFAICT.

AFAICT:

1. \Drupal\Tests\experience_builder\Kernel\Plugin\ExperienceBuilder\ComponentSource\FallbackInputTest::testFallbackInputCanBeRecovered() is now pretty much a duplicate of \Drupal\Tests\experience_builder\Kernel\Plugin\ExperienceBuilder\ComponentSource\SingleDirectoryComponentTest::getPropsForComponentFallbackTesting() — down to the same alt that is being asserted!
2. We only need to move assertions such as self::assertCount(1, $crawler->filter('img[alt="An image so amazing that to gaze upon it would melt your face"]')); into a new abstract method that each ComponentSourceTestBase should implement, because then we could call that method after this assertion at the end of ::testFallback()
   

       // Should be no fallback container.
       self::assertCount(0, $out->filter('[data-fallback]'));
       foreach ($slots as $slot) {
         // Children should still render in the slots.
         self::assertCount(1, $out->filter(\sprintf('h1:contains("This is %s")', $slot)));
       }
   

   👆 that tests after fallback recovery the A) absence of the fallback container, B) the presence of the slots, so adding C) presence of rendered props to that would make it complete.

I belive that this would benefit the overall maintainability.

Re #15 this test actually provides additional test coverage for when you update another component in the preview.

Because when that happens the source plugin's ::clientModelToInput method is called and that changes the stored values.

For example, when a stored component that previously used a generated source plugin (Sdc, JS) is stored in autosave it has a source and resolved key, these come from it's ::inputToClientModel method.

When we call ::patch we rebuild the layout and that calls ::inputToClientModel on a different component, but is passed the old client model from autosave.

So we still need this test, as it covers that scenario

Actually, I think the new approach in 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active makes this redundant. Because conversion happens immediately, so the data stored in the auto save store cannot get out of date w.r.t the source plugin/version.

Postponing on that.

👍 Thanks!