Container compile crash when a service decorates a destructable service

An un-fun way to solve this, which we should avoid, is also: adding the making the service interface (such as ModuleHandlerInterface implement DestructableInterface.

If we determine a service must now implement separate interfaces, then I think 📌 Allow needs_destruction services to run on page cache hits Needs review needs a changelog entry.

In the case of \Drupal\Core\Extension\ModuleHandler::writeCache, should this method be deprecated and removed from the interface?

Should we be doing an instanceof check for destructable interface in DrupalKernel::terminate()?

And/or should hux be removing the terminate tag from the service definition?

@catch I'm wondering if theres a way to just call destruct on the original.

Thinking of Symfony concepts when utilising decorators.

If a service has `calls` applied to it, it doesnt matter if it decorated or not, the calls are always applied to the original/inner service, not the outermost service.

E.g, given:

services:
  dpi.test:
    class: Drupal\dpi\TestService
    calls:
      - [ foo, ['foo!']]

  dpi.deco:
    class: Drupal\dpi\Deco
    decorates: dpi.test
    arguments:
      - '@dpi.deco.inner'
    calls:
      - [ bar, ['bar!']]

At no point will an attempt to call dpi.deco::foo() happen.

Any reason hux can't implement destructable and just call the inner method from its implementation?

Something like this for hux (1.3.x branch) seems to work

#7 was my first thought, but it would probably be good to do something in core so it doesn't crash, even if that's an instanceof check before calling the destruct method.

I think the MR will work but the services are meant to be destructible for a reason, maybe we should issue a warning/deprecation when we find a non DestructableInterface service so downstream users are notified?

Doh it's because the classes are in the same namespace so we don't need a use statement at all. Thanks @longwave.

Pushed up a second branch with another idea which I think is closer to what @dpi is asking for - if we autoconfigure the tag for services with the interface, then decorating services won't get the tag, but inner services still would. However, it's not actually working. We do have one example of autoconfiguring tags in core, but I've never tried it before, hopefully a similarly silly mistake.

I had a play with

$container->registerForAutoconfiguration(DestructableInterface::class)
-      ->addTag('needs_destruction');

and didn't get anywhere. Maybe every services.yml needs to specify autoconfigure: true for even this to work? I guess we could do that but it feels like a bigger change than the scope here.

Instead, I'm iterating over the service definitions where we collect the tagged services, and adding the tag to them there, this seems to work (at least the one test I'm running locally works, as did some manual testing). This was @longwave's suggestion in slack as one of several possible options.

Maybe we can do this to un-fatal things then open a follow-up to use autoconfigure more?

#15 strikes me as being the same root cause as 🐛 Adding media library openers use autoconfigure and tags in 10.3.x has BC consequences Active so I think worth seeing if we can solve both problems with one solution.

I think we might need to just do the quick fix here tbh. Bumping priority since a couple more contrib have reported this now. https://drupal.slack.com/archives/C1BMUQ9U6/p1716826820826669

Thanks at all tracking this so quick and raising prio. Contacted some bigger contrib checking against 10.3 beta to be aware of it. @catch: should we guide here for a quick fix? Is the green MR on top applyable to get rid of WSOD or is there still more to do for the quick fix?

Added Domain to summary as a big contrib affected by this, since this will not only break one but all sites (domains) in the project.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Tested https://git.drupalcode.org/project/drupal/-/merge_requests/7959.diff on test 10.3 beta install with Domain.

The website encountered an unexpected error. Try again later.

Error: Call to undefined method Drupal\domain\DomainListBuilder::formBuilder() in Drupal\domain\DomainListBuilder->render() (line 318 of modules/contrib/domain/domain/src/DomainListBuilder.php).

Drupal\Core\Entity\Controller\EntityListController->listing()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 638)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 53)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle() (Line: 53)
Asm89\Stack\Cors->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 739)
Drupal\Core\DrupalKernel->handle() (Line: 19)

I'd suggest to revert 📌 Allow needs_destruction services to run on page cache hits Needs review . The justification for that change isn't that convincing, and the risk assessment expressed in the CR clearly missed the effects on contrib.

The website encountered an unexpected error. Try again later.

Error: Call to undefined method Drupal\domain\DomainListBuilder::formBuilder() in Drupal\domain\DomainListBuilder->render() (line 318 of modules/contrib/domain/domain/src/DomainListBuilder.php).

This looks completely unrelated.

@znerol that might help with the 2-3 modules affected here, but it won't with similar issues like 🐛 Adding media library openers use autoconfigure and tags in 10.3.x has BC consequences Active .

This looks completely unrelated.

@catch: Yes, it is... wrong issue, sry (mixed tabs, removed comment to prevent confusion)

Yes, the domain issue here is actually https://www.drupal.org/node/3425054 → -- though we still need to check these decorations:

  domain.route_provider:
    class: Drupal\domain\Routing\DomainRouteProvider
    decorates: router.route_provider
    decoration_priority: 10

  domain_config.library.discovery.collector:
    decorates: library.discovery.collector
    class: \Drupal\domain_config\DomainConfigLibraryDiscoveryCollector

  domain_config_ui.factory:
    class: Drupal\domain_config_ui\Config\ConfigFactory
    decorates: config.factory

Those should be two issues in the Domain queue.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

catch → changed the visibility of the branch 3441010-autoconfigure to hidden.

Hiding the autoconfigure branch.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

In the case of module handler, even if without changes from 📌 Allow needs_destruction services to run on page cache hits Needs review , decorators that don't implement DestructableInterface will also fail in 10.2 if we apply 📌 Replace RequestCloseSubscriber with needs_destruction tag on ModuleHandler Fixed .

this just because the call changed from writeCache which declared in ModuleHandlerInterface to destruct which is new implementation.

As for 📌 Allow needs_destruction services to run on page cache hits Needs review , I don't think we can access decorated/original service from DrupalKernel, so even if we register the original service in kernel.destructable_services (eg hux.module_handler.inner) we won't be able to get it from the Kernel with $this->container->get('hux.module_handler.inner').

Perhaps, instead of a parameter we can create a service that can receive the original service as the argument, but that will initialize the instance and defeat the purpose of $this->container->initialized($id).

I can't see any other solution than:

• Implement DestructableInterface from the service's interface, not implementation, this will enforce decorators to implement that also.
• Let it be the decorator's responsibility to implement DestructableInterface, but the container will crash if it is not.

Discussed with @alexpott at Dev Days Burgas.

We agreed that option 1 from #30 is the correct thing to do here. Under the interfaces BC policy → we are allowed to add methods to interfaces in minors where there is a 1-1 relationship with an implementation, which there is here with ModuleHandler.

We also think that this can be implemented in a 10.3.x patch release, given it is no more broken than it is already if someone is replacing the implementation of ModuleHandler with an alternative version that only implements ModuleHandlerInterface without implementing DestructableInterface as well.

longwave → changed the visibility of the branch 3441010-container-compile-crash to hidden.

Ran into this when i had a site with hux:1.4 on core:10.3. Updating to hux:1.5 fixes the site problem.

As for MR !8558:

I reviewed the code and can confirm that it trivially implements the #30.1 proposal (single extended interface).

Requiring a test for this imho is only boilerplate.

Behavior-wise: I can confirm that adding this patch changed the core:10.3+hux:1.4 error like below:

Before:

$ drush cim
 [notice] There are no changes to import.
PHP Fatal error:  Uncaught Error: Call to undefined method Drupal\hux\HuxModuleHandler::destruct() in /home/merlin/Code/geeks4change/sites/site-h4d-greenopolis/web/core/lib/Drupal/Core/DrupalKernel.php:723

After:

$ drush cr
PHP Fatal error:  Class Drupal\hux\HuxModuleHandler contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Drupal\Core\DestructableInterface::destruct) in /home/merlin/Code/geeks4change/sites/site-h4d-greenopolis/web/modules/contrib/hux/src/HuxModuleHandler.php on line 21

Thus RTBC-ing.

FYI but out of scope here: For my own needs i use Composable Inheritance to avoid (amongst others) this kind of pain on highly self-coupled classes that are a pain to decorate but nice to inherit.

Discussed with @catch and @longwave - we agreed to make this change because whilst it does not fix the issue it is a better break and help module maintainers fix it quickly.

Committed and pushed e82372379d to 11.x and 1dd699f07a to 11.0.x and b8241d8c6d to 10.4.x and d0f39b40d2 to 10.3.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.