Handle components provided by ComponentSources IMPLICITLY disappearing: gracefulness when developing SDCs: SDCs may appear/disappear from one request to the next

This is a standard part of the process so I'm changing this to a bug. In the meanwhile, it would be helpful if we can provide @kristen pol some guidance on how to workaround this until we have a proper solution for this.

This is not a bug. Switching git branches and hence suddenly having different git branches is the equivalent of running Drupal with module X installed, switching git branches, and the other git branch not having that module present in the codebase.

Solution: two parts

That being said: yes, we need to guard against this. We need to distinguish between:

• Extensions providing SDCs getting uninstalled (i.e. the "site builder" scenario, not the "developer" scenario): if there's zero component usage, then this is fine/safe, otherwise, we must refuse the extension getting uninstalled. This is the "component present in tree" equivalent of #3452848: [PP-1] Test coverage to prove configurable fields cannot be deleted from content entity types if they are used in XB Content Type Template (which does "field type present in props".
• Extensions providing SDCs changing from one request to the next (i.e. the "developer" scenario). TBD, but I think detecting whether development mode is on (see https://www.drupal.org/node/3359728) and if then this exception is thrown, we should auto-delete on-the-fly and use \Drupal::messenger() to inform the developer with a warning.

I defer the exact solution to @f.mazeikis, this is just my 0.02 😊

Unblocking this workflow TODAY

• ❌ Temporary work-around: vendor/bin/drush config:delete experience_builder.component.starshot_demo+old-starshot-one-col — but this is one-by-one 😬
• ✅ Temporary work-around:
  

  vendor/bin/drush sql:query "DELETE FROM config WHERE name LIKE 'experience_builder.component.%'"
  vendor/bin/drush cr
  

This is closely related to #3464036: Component config entity should validate that the SDC actually (still) exists.

Today during my testing i observed this issue is still present, workaround from #3 did the fix for me.

@lauriii reported a duplicate at #3473770: Error after updating a theme and @guptahemant spotted that. Crediting both.

@longwave opened an issue for essentially the same problem: #3522164: Handle update and delete of SingleDirectoryComponent `Component`s, plus missing config dependencies. I'll let him decide which of the two issues to keep around :) Or maybe he wants to repurpose this for a more explicit DX enhancement, such as a \Drupal::messenger()-powered warning for SDC developers when Twig dev mode is on? 🤓🤔

#3519168: Handle components provided by ComponentSources EXPLICITLY disappearing — enables deleting JS components that are in use might solve most of this.

Contrasting this with #3519168: Handle components provided by ComponentSources EXPLICITLY disappearing — enables deleting JS components that are in use's updated title.

Making it very explicit how this relates to #3519168: Handle components provided by ComponentSources EXPLICITLY disappearing — enables deleting JS components that are in use.

#3519168: Handle components provided by ComponentSources EXPLICITLY disappearing — enables deleting JS components that are in use is in!

The config dependencies part is definitely already taken care of. This is about a module/theme containing an SDC one moment, and not the next.

Closed #3544650: Component definitions not updating when clearing cache in favor of this.

There I created an MR that would "solve" the problem for SDC. That one takes a drastic approach: if a component is not found when rebuilding, it will just delete the component entity.

In #3 this was mentioned as a potential approach if (and only if) the development mode is set.

But here the IS mentions the 3 Canvas-provided cases (blocks, SDCs and even JS components) scenarios.
We cannot rely on this happening only when in development mode. Not best practice, but a block plugin, or an SDC could disappear in a new release of a contrib/custom module/theme, so this wouldn't be only happening in a development scenario.

I wonder if we should:

a) Check if in development: then just delete the component and warn the user.

b) If not:
That's the real complex case. I'd suggest we implement an on-the-fly upgrade process (for existing content) that would convert those to the fallback component keeping its inputs/settings. That's something that was mentioned already for e.g. migrating from layout builder or paragraphs, so it's infrastructure we want to create anyway for any potential component source.

But we still would need to delete the component, as even disabling the component requires the e.g. SDC to exist (is verified on save). If we think this is too drastic, then we would need to try/catch everywhere in canvas and assume that the missing component on discovery is a valid state for a project.

Transferring beta blocker tag from the duplicate issue.

Beta blockers are by definition also stable blockers, but removing the latter tag in order to make it easier to scan a queue of stable blockers that aren't also blockers to beta or RC.

The workaround in #3 (adjusted for Canvas) was no longer working for me, so I took a crack at it. It looks to me like the addition of folders caused the workaround to stop working, so here's an updated fix for the current build, which is 1.0@alpha at the time of writing.

vendor/bin/drush sql:query "DELETE FROM config WHERE name LIKE 'canvas.component.%' OR name LIKE 'canvas.folder.%'"
vendor/bin/drush cr

1. Create a Page with a component tree that contains the canvas_test_sdc:my-hero SDC
2. I enabled Twig development mode at /admin/config/development/settings
3. Rename the SDC — note that ONLY renaming the directory has no effect, one must rename everything: the directory, the CSS, the JS, the *.component.yml and the Twig file.
4. At /page/1: no hard crash, just a message that the Twig template is not found:
5. Explicitly wipe the SDC cache by deleting the component_plugins key-value pair in the cache_discovery table. Now you do get the reported fatal error with stack trace:
   

   The website encountered an unexpected error. Try again later.
   
   Drupal\Core\Render\Component\Exception\ComponentNotFoundException: Unable to find component "canvas_test_sdc:my-hero" in the component repository. [The "canvas_test_sdc:my-hero" plugin does not exist. Valid plugin IDs for Drupal\canvas\Plugin\ComponentPluginManager are: olivero:teaser, canvas:image, ✂✂✂✂✂✂, canvas_test_sdc:my-herod, sdc_test_all_props:all-props] in Drupal\Core\Theme\ComponentPluginManager->createInstance() (line 123 of core/lib/Drupal/Core/Theme/ComponentPluginManager.php).
   Drupal\Core\Plugin\DefaultPluginManager->getDefinition('canvas_test_sdc:my-hero') (Line: 16)
   Drupal\Core\Plugin\Factory\ContainerFactory->createInstance('canvas_test_sdc:my-hero', Array) (Line: 85)
   Drupal\Component\Plugin\PluginManagerBase->createInstance('canvas_test_sdc:my-hero', Array) (Line: 107)
   Drupal\Core\Theme\ComponentPluginManager->createInstance('canvas_test_sdc:my-hero') (Line: 144)
   Drupal\Core\Theme\ComponentPluginManager->find('canvas_test_sdc:my-hero') (Line: 114)
   Drupal\canvas\Plugin\Canvas\ComponentSource\SingleDirectoryComponent->getComponentPlugin() (Line: 141)
   Drupal\canvas\Plugin\Canvas\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase->getMetadata() (Line: 412)
   Drupal\canvas\Plugin\Canvas\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase->requiresExplicitInput() (Line: 317)
   Drupal\canvas\Plugin\Canvas\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase->getExplicitInput('ea85032c-0b33-4962-81f2-7f83c6b25046', Object) (Line: 446)
   Drupal\canvas\Plugin\Field\FieldType\ComponentTreeItemList->getHydratedValue() (Line: 354)
   Drupal\canvas\Plugin\Field\FieldType\ComponentTreeItemList->getHydratedTree() (Line: 191)
   Drupal\canvas\Plugin\Field\FieldType\ComponentTreeItemList->toRenderable(Object) (Line: 28)
   Drupal\canvas\Plugin\Field\FieldFormatter\NaiveComponentTreeFormatter->viewElements(Object, 'en') (Line: 91)
   Drupal\Core\Field\FormatterBase->view(Object, 'en') (Line: 275)
   Drupal\Core\Entity\Entity\EntityViewDisplay->buildMultiple(Array) (Line: 341)
   Drupal\Core\Entity\EntityViewBuilder->buildComponents(Array, Array, Array, 'full') (Line: 283)
   Drupal\Core\Entity\EntityViewBuilder->buildMultiple(Array) (Line: 240)
   Drupal\Core\Entity\EntityViewBuilder->build(Array)
   …
   

Should be caught/handled by the protection layer #3485878 introduced?

Now, why doesn't #3485878: Server-rendered component instances should NEVER result in a user-facing error, should fall back to a meaningful error instead (+ log) catch this? Because if that were to happen, we'd have the same graceful degradation as we have everywhere else:

(Which is IMHO the correct solution, I'd suggest we implement an on-the-fly upgrade process (for existing content) that would convert those to the fallback component keeping its inputs/settings. from #20 is impossible without knowing what the ID of the SDC is — multiple could be changing at the same time, and surely we don't want to be guessing automatically? 🫣)

🕵️ So: why not?

The stack trace in the previous comment provides a hint: the crash happens not during actual rendering, but during the collecting of explicit inputs.

The point where the rendering starts is ::toRenderable(). This is a simplified representation of that:

  public function toRenderable(ComponentTreeEntityInterface|FieldableEntityInterface|null $entity = NULL, bool $isPreview = FALSE): array {
…
    $renderable_component_tree = $this->getHydratedTree();
…
    return self::renderify(self::buildRenderingContext($this, $entity), $hydrated, $isPreview);
  }

The RenderSafeComponentContainer stuff happens in ::renderify(). But you can see in the stack trace, that the crash happens before that point: it happens in the ::getHydratedTree() call.

💡Attempt #1

So then the naïve solution is: make sure that collecting of inputs happens within the same protection layers.

Trying! 🤓

Been pairing with @penyaskito for the past 2 hours. We're doing something what I described in my last comment, but different: rather than adding protection layers to the collecting of inputs, we're instead relying solely on information in the Component config entity (which is supposed to be the source of the current truth, NOT the filesystem, precisely for situations like this, where the world is changing underneath us!).

That got us much further! 🤘

Now you see:

aka the infra that #3485878: Server-rendered component instances should NEVER result in a user-facing error, should fall back to a meaningful error instead (+ log) added! 🥳

We've also gotten the /canvas/api/v0/config/component API response working again. It now provides the UI with a flag that indicates whether the known component should be available for instantiation (broken: false) or not (broken: true). That should also change the icon in the component list to something conveying brokenness.
Tagging Needs followup to build the UI pieces for that.

Next up: making the component instance form use the "fallback" form, without actually modifying the Component config entity to switch to the fallback source — that'd be much too disruptive/annoying while developing.

Paired for another hour with Wim.

We now have fixed the component instance form to use the "fallback" form, see attachment.

We identified some other ways the reality and the component entity metadata can differ (including not only SDC and blocks, but also JsComponents), and we need to take care of that too.

We also identified that we are missing component metadata in versioned components: what's required and what not. So a follow-up will be created which requires an upgrade path (and new versions yay!)

Follow-up created for #26: #3548922: A component appearing in the list of components to instantiate that is broken during development should convey that.

[…] making the component instance form use the "fallback" form, without actually […]

@penyaskito made this work — see his screenshot from #28:

We've now got a — we think, CI will tell 😅 — complete solution for the scope of this issue: gracefully handling of SDCs (dis)appearing from one request to the next, i.e. while developing. And it's built in a ComponentSource-agnostic way, so it will work for the block, js etc. sources too.

Test coverage

Still missing: test coverage. Proposed approach: new ComponentSourceTestBase::testBroken() test. This:

• will force every ComponentSource plugin to test it 👍 — yay thanks #3517966: Failing kernel tests for all ways a component source can fail to render on the server side!
• should create a Page with a component tree that includes a single component instance of a Component in the tested ComponentSource. At this point ::validate() must pass. Do NOT call ::save() yet.
• load the Component, append _renamed to its ID, do NOT ::validate() (because that would fail — we're trying to simulate this scenario!), but DO call ::save(). This is now simulating in tests that the underlying component (whether it's a block plugin, SDC or code component) that actually the developer has just renamed things, causing the component to not exist.
• Now update the component tree in the Page to refer to the [original]_renamed Component by modifying the component_id field property. This must be saved, but validation should pass too: while the Component is invalid (it is out of sync with reality), the Page is valid. Just like reported in this issue.
• Test failing in HEAD, passing here: when requesting $page->url(), it should render what's visible in #25/#26.
• Test failing in HEAD, passing here: /canvas/api/v0/config/component should result in a 200 (500 in HEAD), and the renamed Component should be listed, it should have the broken: true flag, and its default_markup should again be what's visible in #25/#26.
• Test failing in HEAD, passing here: the component instance form should result in a 200 (500 in HEAD), and should show what @penyaskito shared in #28

Out of scope: handle more breakage scenarios when developing components

There's more things that can fundamentally break a component instance though:

1. ✅ (this issue) the underlying component disappeared from the ComponentSource (whether deleted or renamed)
2. ❌ the set of required explicit inputs increased (SDC terminology: "props") — decreasing is not a problem!
3. ❌ the set of required explicit inputs changed (SDC terminology: "props") aka a required explicit input was renamed
4. ❌ the shape of an explicit input (SDC terminology: "props") changed, e.g. a type: string becoming a type: integer

⚠️ Currently, there's also ways to break things in Canvas' UI that are not fundamental, such as passing an optional prop that either no longer exists or has been renamed. (That currently triggers the validation in \Drupal\canvas\Plugin\Canvas\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::getDefaultStaticPropSource() but arguably should not. See @heyyo's #3532514-18: Gracefully handle components in active development: ensure great DX.)

(This may cause garbage values though, for that we have #3524401: `GeneratedFieldExplicitInputUxComponentSourceBase::validateComponentInput()` allows garbage to pile up.)

👆 To fix that: #3532514: Gracefully handle components in active development: ensure great DX, for which I'm updating the issue title + summary for to more clearly reflect what it's about; should be actionable once this MR lands. → updated: #3532514-23: Gracefully handle components in active development: ensure great DX.

assuming @penyaskito is asleep I'll push a rough skeleton of the test in #30 so I can expand on it in #3532514: Gracefully handle components in active development: ensure great DX

Pushed the tests per #30 - I don't think they make sense for JSComponents or Personalization so marked them as skipped. Went with a simpler approach than renaming because I think we'll need to alter definitions further in #3532514: Gracefully handle components in active development: ensure great DX and this plumbing lets us do that.

Fixed phpstan while I was at it.

Removing myself from the assigned field because with the basis of these tests in place I can expand on other cases in #3532514: Gracefully handle components in active development: ensure great DX, setting it back to @penyaskito

Pushed two extra commits to a new branch in 3470422-be-nice-for-sdc-developers-larowlan

I'm planning to use that approach in #3532514: Gracefully handle components in active development: ensure great DX

Also includes tests (that will fail with the current code in the branch for this issue) for when the missing component has slots and there are child components in those slots.

To resolve that I was forced to go with the approach in 3470422-be-nice-for-sdc-developers-larowlan, which is why I think we need that here. It is a best-effort attempt but without the update path we'll need for #3532514: Gracefully handle components in active development: ensure great DX

Paired with @penyaskito, and:

• we now both fully understand how @larowlan's BlockComponent changes work, and we added comments to help the next person understand it too 👍
• we got JsComponentTest::testIsBroken() unskipped; we can simulate this problem occurring even for code components despite all protection layers, by directly manipulating config.storage

Green, so merging! 🚀

And it's in!

See y'all in #3532514: Gracefully handle components in active development: ensure great DX next 😊

Verified below scenarios, @wim-leers, please check failed test cases: TC03 and TC08

NW for Mayur's findings.

The original MR was merged before beta, but tracking the newly discovered failures in #42 as an RC blocker. We don't necessarily need to fix them before RC, but we should determine what the cause of the errors is and then make a determination based on that whether or not to hold up an RC on fixing them.

To address TC03 + TC08, we need to do manual testing aka investigate what's happening. Any takers? 😊

I've had a look at TC03 & TC08 re #42 - I couldn't seem to reproduce these just based on loading the page and an error dialog appearing with the mentioned errors - I maybe misunderstanding.

But I did encounter this which I thought was related to TC03. If the code component does not have any props, if I have a component that does have props then I get the fallback as expected.

The full error I get is this error in the component settings sidebar:

Error 500: Drupal\canvas\Plugin\Canvas\ComponentSource\Fallback::buildComponentInstanceForm(): Argument #5 ($inputValues) must be of type array, null given, called in /var/www/html/web/modules/contrib/canvas/src/Form/ComponentInstanceForm.php on line 143

So at this point $inputs in the ComponentInstanceForm is NULL - it looks like we account for specifically if a BlockComponent source has inputs that are NULL.

      // For blocks, the client model is invalid, because $props is the "undefined" string.
      // So let's get the data from the stored tree (better than nothing)
      // @todo We require to harden this in the client-side.
      if ($inputs === NULL && $component->getComponentSource() instanceof BlockComponent) {
        $inputs = $stored_tree->getComponentTreeItemByUuid($component_instance_uuid)?->getInputs() ?? [];
      }

From my testing it looks like with any component source the inputs can be NULL at this point if there are no props defined, and I'm seeing what is mentioned in the code comment "the client model is invalid, because $props is the "undefined" string" so I'm not sure if that check should/needs to be so specific?

Caught up with @mayur-sose just to clarify and he showed me TC03 - it is exactly as his steps mention, I was saving the page when testing.

TC08 - we couldn't reproduce at the time but @mayur-sose will clarify those steps if he can reproduce this again.

I've still not be able to reproduce TC03, following the steps and discussed with @mayur-sose, both of us on the latest 1.x with this MR. He is seeing:

What I'm seeing:

So I think it maybe a good idea if someone else could also attempt to reproduce this and what they get?

Clarified steps for TC08:

- Add an SDC component to a page
- Rename the SDC component
- Clear the cache
- Reload the page
- Try to add the SDC again to the canvas

I think that this sounds like what is described at #26 and will be handled in a follow-up #3548922: A component appearing in the list of components to instantiate that is broken during development should convey that

I tried to test TC03. This is with component that has no props

This is what I see for a page in the editor

I am able to delete the code component by using the layers menu

I also tried it with a published page

TC03 with a component with props

I will look at the error if there is no props

For TC08 the UI does not become unusable but there error in the php log is not very helpful

AssertionError occurred during rendering of component 520bacb3-82a3-4d9e-a81a-cdc7232937c6 in Page Untitled page (-), field components: assert($js_component instanceof JavaScriptComponent)

It does not contain the ID of the component that is missing

I pushed an MR that addresses TC03 and TC08

For TC08 the error message now contains the ID of the component

Drupal\Core\Render\Component\Exception\ComponentNotFoundException occurred during rendering of component 520bacb3-82a3-4d9e-a81a-cdc7232937c6 in Page Untitled page (-), field components: The JavaScript Component with ID `noprops` does not exist.

For TC03, it makes sure the input is an empty array not NULL so there is not an exception, but we might want to change the error message in this case. See MR comment

oK chatted with mayur-sose and I know what the difference

I was doing
./vendor/bin/drush cdel canvas.js_component.NAME
So deleting the js_component config

@mayur-sose was doing
./vendor/bin/drush cdel canvas.component.js.NAME
So deleting a different thing.

I guess @mayur-sose's doing what described in the issue summary. seems like we should handle both?

Thanks, @tedbow! Per #44, this was tagged as "RC blocker" until we knew what the issues were. Now that we do, I'm downgrading it to "RC target". I think the current MR is great, so if other reviewers agree, let's merge it and make the RC that much better, but if reviewers spot problems with it, we don't need to hold up the RC on it.

To summarize:

• #42.TC03 happens with a Drush delete of the component entity, not the component's source (Twig file, Block plugin, or js_component entity). I don't think that needs to be RC blocking: people aren't as likely to be deleting the Component entity during component development, they're more likely to be deleting component sources. But we should open a follow up to fix this after RC.
• #42.TC08 was fixed in #3548922: A component appearing in the list of components to instantiate that is broken during development should convey that.
• #46 and #51 are fixed by the current MR. Let's merge it if it looks good and not likely to cause other problems.

Added follow-up #3553620: Prevent or gracefully handle deletions (e.g., via Drush) of a Component config entity for #42.TC03

Is there any way we can write test coverage for this?

We have existing examples in core that create modules on the fly during the test - see \Drupal\Tests\system\Functional\Theme\MaintenanceThemeUpdateRegistryTest::prepareEnvironment we could write a test module to $this->siteDirectory . '/modules/' that has a block plugin, and then delete it and assert the expected behaviour

@larowlan see changes to `tests/src/Kernel/ComponentInstanceFormTest.php`, it should add the necessary test coverage for a block disappearing.

Added some tests that demonstrate the 500 errors I was getting in manually testing yesterday

They fail without the latest changes

This is ready for review again

RTBC. Lee to confirm all threads can be resolved.

@larowlan resolved all threads, so unassigning him.

I'll manually test and hopefully merge this tomorrow my morning.

There's 3 parts I didn't understand at first.

1. ✅ One part (the hardening in JsComponent) I was able to figure out, and I left an explanation for the next person: https://git.drupalcode.org/project/canvas/-/merge_requests/220/diffs#not...
2. ✅ Another part (changes in client model conversion) I was also able to figure out: https://git.drupalcode.org/project/canvas/-/merge_requests/220/diffs#not... — and it means this MR !220 is essentially a missing child issue for #3517941: [META] Robust component instance error handling, and #3548922: A component appearing in the list of components to instantiate that is broken during development should convey that didn't account for this edge case.
3. 🤔 But another part I wasn't able to figure out, and the test-only CI job also says that some of the new tests are passing: https://git.drupalcode.org/project/canvas/-/merge_requests/220#note_626073. What am I missing?

Needs follow-up for

• https://git.drupalcode.org/project/canvas/-/merge_requests/220/diffs#not...
• which is just repeating what @penyaskito and I pragmatically did on October 1 to get RC1 out the door in the other MR that this issue landed, which is working around a client-side bug: https://git.drupalcode.org/project/canvas/-/merge_requests/142/diffs#not...

I think the questions at #66 were resolved in the MR, and the tests improved to match the reality of what the query args to the component form instance.

IMHO all open MR threads should remain open for archeology purposes.

Marking as RTBC assuming all tests come green, and assigning to Wim for confirmation.

We need a follow-up for investigating

why the client sends form_canvas_props: undefined, instead of an actual JSON blob. (When the fallback source is in use, the client should send the model it has for this component instance, not the values for the component instance form.)

❤️🥳

Thanks so much for finishing this, @penyaskito! Only did some trivial refactoring to avoid repeating non-trivial logic in 3 places 👍

D'OH! I once again mixed this issue up with #3532514: Gracefully handle components in active development: ensure great DX, this time messing up the entire issue summary 🙈

Restored.

Filed the necessary follow-up: #3558721: Regression: can no longer access previously stored values for block component instances.

Looks like #3532514: Gracefully handle components in active development: ensure great DX having just landed has an interesting interaction with this MR, causing JsComponentTest::testIsBroken() to fail.

I've done plenty of manual testing while ironinig out the last problems I spotted.

RTBC per https://git.drupalcode.org/project/canvas/-/merge_requests/220#note_627277

Thanks, everyone!

Suggested commit message:

feat: #3470422 Handle components provided by ComponentSources IMPLICITLY disappearing: gracefulness when developing SDCs: SDCs may appear/disappear from one request to the next

By: @mayur-sose
By: @tedbow
By: @mglaman
By: @larowlan
By: @penyaskito
By: @wimleers

Fixed! thanks everyone!

This introduced consistent CI failures for the MySQL + PostgreSQL PHPUnit CI jobs:

This regressed: #3558721: Regression: can no longer access previously stored values for block component instances. E2E tests would've caught this.