SDC *.component.yml metadata is cached aggressively, gets in the way of component development

This also ties in to Experience Builder: #3463999-9: [PP-1] Auto-generate Component config entities for all discovered SDCs that meet XB's minimum criteria → .

Hi Wim,

I would prefer we do not introduce a new mechanism (in settings.php or in config with form in some admin page...) specific to SDC, so proposal 3 looks the best to me.

Fyi, for XB component development, I use this from Wim regularly :)

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

Here is a starting point proposition for case 3.

I would like to add tests but it seems there is only Kernel tests at the moment for component, a Unit test would perhaps be better.

So is consensus to use #3? So we can update the remaining tasks so it's clear what's being proposed here.

Test failures appeared to be random.
Applied a super nitpicky return type as a new function.
Tagging for tests

I've reordered the steps in ComponentPluginManager::getDefinitions() to avoid discovering the definitions twice and unnecessarily caching them.

Additionally, I've added a kernel test to cover this functionality.

Fix looks good, test coverage was added.

This looks good, basing it on the twig development settings seems fine. I think we might want to consolidate some of this on ✨ Add a Production/Development Toggle To Core Needs work but this doesn't make that any harder or easier.

Committed/pushed to 11.x, thanks!

catch → closed merge request !9551

Hey all 👋. I started noticing a PHP WSOD today and after some xdebug'ing, traced it back here:

TypeError: Drupal\Core\Plugin\DefaultPluginManager::doGetDefinition(): Argument #1 ($definitions) must be of type array, null given, called in /var/www/html/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryCachedTrait.php on line 25 in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 43 of core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).

To reproduce:

• Install Drupal standard profile
• Login and enable Twig development mode
• Visit a page that uses an SDC, eg. front page of the site, or any Olivero page. You may need to rebuild cache if you had already visited this page before turning on Twig development mode
• 💥 Above error

Reviewing the MR, the overridden getDefinitions method does not call the parent method when Twig debugging is on. What appears to be missing in relation to the parent is a call to $this->setCachedDefinitions, which in turn sets $this->definitions, which is depended in in the place where the error occurs.

Hopefully that's helpful.

Whoops! I will work into this tomorrow ASAP.

Filed as a bug: 🐛 Twig development mode breaks pages with SDC Active

Reverted for now.

Thanks for filing an issue @m4olivei, I was wondering if that was the correct approach, rather than re-opening this one. But since this has been reverted and changed to NW, I will work on it.

I will also try to figure out how we could have prevented this bug to be overlooked and avoid it in the future.

Thanks for filing an issue @m4olivei, I was wondering if that was the correct approach, rather than re-opening this one.

I wondered that as well. Since this one has been re-opened, I'll close the one I opened. Feel free to pull from the MR I opened over there if its useful.

vidorado → changed the visibility of the branch 11.x to hidden.

After digging deeper into the code, I realized that we were relying solely on the development settings configuration form, but not on the development.services.yml file configuration. This initially puzzled me when I tried to reproduce the issue from #18.

So, I've updated the code to rely on the container's twig.config parameter, ensuring that we honor the actual configured value in both cases.

Regarding the WSOD reported also in #18, I've also set the $this->definitions static cache in ComponentPluginManager::getDefinitions(). Additionally, I updated ComponentPluginManagerTest to modify the container's twig.config parameter, to rely on a more realistic behavior—checking whether definitions are not refreshed when they change, rather than depending on whether the cache backend is called or not.

About we could have catched the bug at first...

I'm not sure what test should be added:

1. A new test in DevelopmentSettingsFormTest that renders an SDC component in debug mode (this seems odd, as it would make Development Settings aware of SDC).
2. A new test in OliveroTest that renders an SDC component in debug mode (makes more sense but feels disconnected from ComponentPluginManager, potentially hiding the reason the test is needed).
3. A new ComponentPluginManagerRegressionTest class to explicitly ensure this issue doesn't happen again (this seems like the best approach to me).

About Berdir's comments above

I believe they have been taken into account. We now understand that we can only add a callback function to a bundle definition to restrict the options or customize the labels, but the allowed_values setting must not be overridden.

Regarding this, I think the only unanswered comment is #11, which asks what would happen if we did override it.

I tested this scenario, and it turns out that it is possible. I created a custom test entity with an options_field that has a default allowed_values setting of ['Foo', 'Bar'], two bundles, and an overridden allowed_values setting of ['Baz'] for the second bundle. I was able to create entities for both bundles, and the values in the options_values field dropdown were displayed as configured, with no errors thrown.

I'm not sure what action, if any, we should take regarding this.

About adding the entity ID to the cache ID

This is a separate question, and I assume you're suggesting adding the entity ID to the cache key because the allowed_values callback function receives the entity as a parameter, meaning it could depend on any part of the entity, not just the bundle.

In that case, I agree that we should add the entity ID to the cache key. It's unfortunate, as this will result in a large number of cache entries—one per entity—but I don't see another way of handling it since the callback function does not return any cacheability metadata.

Please confirm that we're all on the same page, regarding to this, and I'll proceed with adding the entity ID to the cache key.

Thanks!