Remove canvas_install() entirely and trigger the installation of canvas_stark unconditionally when Canvas itself is installed; forbid the use of `drupal_flush_all_caches` in Canvas's code base

I'm sorry to say that this broke Drupal CMS completely. It is not installable, at all, with Canvas 1.3.0.

The problem is the call to drupal_flush_all_caches() in canvas_install(). This causes the request stack to be recreated without the current request, which means the session is unavailable -- and that, in turn leads to a \Symfony\Component\HttpFoundation\Exception\SessionNotFoundException being thrown by \Symfony\Component\HttpFoundation\RequestStack::getSession() if anything even inadvertently calls it -- in our case, it's because Automatic Updates tries to get the shared tempstore, which needs to examine the session during initialization.

But, I hear you saying, the drupal_flush_all_caches() call is in 1.2.0 also, and that was working!

Well...no it wasn't. It was in fact being masked by an early return if $is_syncing was TRUE, which it is while applying recipes (which is 95% of what the Drupal CMS installer does). That early return was removed in 1.3.0.

Since this is the eve of Drupal CMS 2.1.0's release, the timing is extremely bad.

We cannot work around this by pinning to Canvas 1.2.0, because the pinned dependency will end up in the top-level composer.json (due to recipe unpacking) and make Canvas non-updateable unless you know to edit composer.json. This is why Drupal CMS never, ever pins dependencies under any circumstances.

There is only one course of action: the $is_syncing early return has to be restored, and then Canvas 1.3.1 must come out as a hotfix release. This is a Drupal CMS release blocker.

To be clear on the urgency of this: I think this is also breaking the current stable release!

In other words, Drupal CMS is not installable, in any current stable version, until we've hotfixed this.

If we can get rid of the drupal_flush_all_caches call, we will be okay and could release another hotfix.

How can we ensure we never need to be surprised by this again?

1. Why did Canvas’ explicit recipe tests not hit this problem? Can we add the necessary test coverage?
2. If we can’t: can we add a CI job with a script that would reproduce this? Sounds like this needs Automatic Updates to do something after Canvas is installed in the same request? 😦

Drupal’s many abstraction layers, each with their own bugs/edge case/caveats and most with poor DX and edge case handling is what is led us to this place.

No, no, no. There is NOTHING Automatic Updates could have done to prevent this, and I can walk you through what happened, over Zoom, if you don’t believe me.

The only reason Automatic Updates is in the blast radius is because it has a hook service which injects another service that, in its dependency tree, requires the shared tempstore factory. The shared tempstore factory is asking for the request, and the container doesn't have it due to the reset Canvas is doing.

I will try to give you a test case for this. I would think that it should be reproducible.

!780 supplies a test which tries to install Drupal using a simple recipe that installs only two modules: Canvas, and a stub module with a no-op hook service that injects the shared temp store. That is, overall, essentially what the Drupal CMS installer is doing: installing Drupal from a recipe.

When you remove the $is_syncing early return from canvas_install, the test dies with a very, very similar exception:

Symfony\Component\DependencyInjection\Exception\RuntimeException: You have requested a synthetic service ("kernel"). The DIC does not know how to construct this service.

/Users/phen/Sites/canvas-dev/vendor/symfony/dependency-injection/ContainerBuilder.php:1099
/Users/phen/Sites/canvas-dev/vendor/symfony/dependency-injection/ContainerBuilder.php:632
/Users/phen/Sites/canvas-dev/vendor/symfony/dependency-injection/ContainerBuilder.php:1315
/Users/phen/Sites/canvas-dev/vendor/symfony/dependency-injection/ContainerBuilder.php:1267
/Users/phen/Sites/canvas-dev/vendor/symfony/dependency-injection/ContainerBuilder.php:1167
/Users/phen/Sites/canvas-dev/vendor/symfony/dependency-injection/ContainerBuilder.php:632
/Users/phen/Sites/canvas-dev/vendor/symfony/dependency-injection/ContainerBuilder.php:1315
/Users/phen/Sites/canvas-dev/vendor/symfony/dependency-injection/ContainerBuilder.php:1267
/Users/phen/Sites/canvas-dev/vendor/symfony/dependency-injection/ContainerBuilder.php:1167
/Users/phen/Sites/canvas-dev/vendor/symfony/dependency-injection/ContainerBuilder.php:632
/Users/phen/Sites/canvas-dev/vendor/symfony/dependency-injection/ContainerBuilder.php:577
/Users/phen/Sites/canvas-dev/web/core/lib/Drupal/Core/DependencyInjection/ClassResolver.php:28
/Users/phen/Sites/canvas-dev/web/core/lib/Drupal/Core/Utility/CallableResolver.php:100
/Users/phen/Sites/canvas-dev/web/core/lib/Drupal/Core/Extension/ModuleHandler.php:742
/Users/phen/Sites/canvas-dev/web/core/lib/Drupal/Core/Extension/ModuleHandler.php:338
/Users/phen/Sites/canvas-dev/web/core/lib/Drupal/Core/Extension/ModuleHandler.php:388
/Users/phen/Sites/canvas-dev/web/core/lib/Drupal/Core/Extension/ModuleInstaller.php:257
/Users/phen/Sites/canvas-dev/web/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
/Users/phen/Sites/canvas-dev/web/core/lib/Drupal/Core/Recipe/RecipeRunner.php:275
/Users/phen/Sites/canvas-dev/web/core/includes/batch.inc:300
/Users/phen/Sites/canvas-dev/web/core/includes/form.inc:834
/Users/phen/Sites/canvas-dev/web/core/includes/install.core.inc:647
/Users/phen/Sites/canvas-dev/web/core/includes/install.core.inc:565
/Users/phen/Sites/canvas-dev/web/core/includes/install.core.inc:122
/Users/phen/Sites/canvas-dev/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:326
/Users/phen/Sites/canvas-dev/web/core/tests/Drupal/Tests/BrowserTestBase.php:554
/Users/phen/Sites/canvas-dev/web/core/tests/Drupal/Tests/BrowserTestBase.php:349

I think that this is a different manifestation of the same bug: when Canvas blows away the container, some crucial services stop working because of the indiscriminate nature of the reset. The test passes if you restore the $is_syncing check, which would seem to confirm that it is masking the bug.

When I paused this test during debugging and examined the call stack, I discovered that this particular exception path is being caused by \Drupal\canvas\Hook\ComponentSourceHooks::modulesInstalled(), because the service is trying to construct \Drupal\canvas\ComponentSource\ComponentSourceManager, which has the kernel as a dependency.

The fact that \Drupal\canvas\Hook\ComponentSourceHooks::modulesInstalled() returns early if config is syncing is of no help (Automatic Updates has this guard rail too), because the problem is happening before the hook is even invoked -- it's crapping out while the hook service is still being constructed by the container.

So it's the same basic cause: a service innocently depends on a service that no longer exists, or has changed state in a breaking way, because the container it thought it had, is now gone.

Playing with this a bit more and I discovered that removing the drupal_flush_all_caches call doesn't resolve the exception the test is turning up.

This still shows that installing Canvas via a recipe at install time breaks without the $is_syncing guard, but why seems a little more ambiguous.

The current band-aid still causes issues for me when installing via recipes. On a site that has entity_embed installed, applying a recipe to install canvas with its config causes this error for me:

PHP Fatal error: Cannot declare class Drupal\entity_embed\Plugin\CKEditor5Plugin\EmbedCKEditor5PluginBase, because the name is already in use in /app/web/modules/contrib/embed/src/Plugin/CKEditor5Plugin/EmbedCKEditor5PluginBase.php on line 44

Update: unrelated issue, somehow this is triggered by canvas now. nevermind. this got fixed in #3517882: The namespace of EmbedCKEditor5PluginBase does not respect PSR4.

Still working on this.

The line that causes the exception in \Drupal\Tests\canvas\Functional\InstallerTest is:

\Drupal::service(ThemeInstallerInterface::class)->install(['canvas_stark']);

...in canvas_install.

@phenaproxima: thanks — this seems quite important to resolve indeed 🙏 Reflecting in issue metadata.

Discussed with @bnjmnm and my feeling is that Claude was correctly identifying "rebuilding the container while rebuilding the container" as the source of a lot of fragility. I implemented an experiement: what if we unconditionally install canvas_stark when Canvas is? We could do this in hook_modules_installed.

Except for one PHPUnit test, all passed. Yes, even the end-to-end ones. Holy 💩!

This lets us remove the entire canvas_install function. I think we should take it one step further, and use PHPCS to forbid all calls to drupal_flush_all_caches(). It's a bad function that should never be used.

All tests should now be passing.

I ended up rewriting \Drupal\Tests\canvas\Functional\UninstallModulePageTest because it is extremely outdated, harkening back to when Canvas was called Experience Builder...and it makes, to be very frank, zero sense as written. It's extending core's InstallerTestBase, which is for testing the UI installer, not the module uninstall page. It is also using outdated configuration which no longer applies. It should never have passed at all, but somehow it has been, essentially by coincidence. I looked at the issue which introduced it (#3464830: FieldTypeUninstallValidator: TypeError: str_starts_with(): Argument #1 ($haystack) must be of type string, null given in str_starts_with()) and I think I'm keeping the intention of the test, which was to confirm that /admin/modules/uninstall can be visited without a fatal error.

I'll need to rewrite the issue summary for this one.

Back to you.

🤩 Fantastic to see this improved!