Allow updating modules with new service dependencies

Looking at the issue summary and thinking about efforts we were making when we were trying to support Drupal 7 to Drupal 8 via update.php - I'm not sure option B is possible. If we add all the services to the service container. Then one of these services might make a change, for example an on demand creation of a table, but the module providing the service might not end up installed. That might be really tricky to get back from.

I think @dawehner might bring up the idea of pre-update hooks where this type of the can be resolved before there is a container that can get in the way. The problem there specifically for module install is that we have hook_install where people use the container.

Update path in Acquia Content Hub is broken because of it. #2851258: Add diff module to the composer.json

We had that kind of discussions/explorations with #2547507: Consider adding hook_update_early_N() support before

Added option C.

One possible workaround would be to do a module exists / lookup in the core.extension configuration inside a ServiceProvider class. If your new dependency doesn't exist, ignore the services, otherwise continue on.

It feels like your module should be written against those problems in the first place, so its not necessarily the worst when it would break and provide you a helpful exception message.

Another option is for people to be able to version their services.yml file. So you build with the old services and then once the new dependency is enabled you switch to the new services.yml file. How might this work?

Timeline:

1. ModuleA is installed and has ServiceA declared in services.yml
2. ModuleA is update to depend on ModuleB and ServiceA depends on ServiceB
3. modulea_update_8001 is added to do the update. It would crash because now modulea.services.yml has a serivce dependent on something from an uninstalled module
4. But ModuleA has a modulea.services.8001.yml that it uses instead whilst (and before) running modulea_update_8001.
5. Immediately after modulea_update_8001 the container is rebuilt and because the schema is not 8001 the service builder uses updated modulea.services.yml and not the special 8001 file.

It a lot of instances the modulea.services.UPDATE_N.yml file might just not include the affected service because it is not used at all during the update.

@bircher
Do you want to summarize the discussion we had at that day?

1. +++ b/core/modules/system/src/Tests/Update/UpdatePathNewDependencyTest.php
   @@ -0,0 +1,46 @@
   +/**
   + * @group system
   

   Missing short description.

2. +++ b/core/modules/system/src/Tests/Update/UpdatePathNewDependencyTest.php
   @@ -0,0 +1,46 @@
   +    // No database required,
   

   Comma at the end of the comment, although a better copy (or not comment at all) I think will help readability.

3. +++ b/core/modules/system/src/Tests/Update/UpdatePathNewDependencyTest.php
   @@ -0,0 +1,46 @@
   +    $this->rebuildAll();
   +    $this->drupalGet('<front>');
   

   Assertion passes locally without this two lines.

4. +++ b/core/modules/system/tests/modules/new_dependency_test/new_dependency_test.install
   @@ -0,0 +1,13 @@
   + * @file
   

   Missing short description.

5. +++ b/core/modules/system/tests/modules/new_dependency_test/src/DependentService.php
   @@ -0,0 +1,22 @@
   +class DependentService {
   

   This class and its method needs a bit of love docblock wise (Class doc, constructor argument, hello() method).

6. +++ b/core/modules/system/tests/modules/new_dependency_test_with_service/src/NewService.php
   @@ -0,0 +1,14 @@
   +class NewService {
   

   Same as above, a bit of doc-love is needed.

Solution A is simple and effective, although I must say I was a big fan of C.
I don't see scenario where Solution A won't work. The only "critical" service which must never fail here is module_installer itself and its arguments, but as core services they will never depend by any module.

Will the container rebuild automatically after the update process is complete?
(I know it does for post_updates, not sure about updates).

@bircher thanks for your work. Just a minor issue:

+++ b/core/modules/system/tests/modules/new_dependency_test/new_dependency_test.install
@@ -0,0 +1,14 @@
+ * Enable the config_filter module.
...
+  \Drupal::getContainer()->get('module_installer')->install(['new_dependency_test_with_service']);

"Enable the new_dependency_test_with_service module."

However

I've done a manual test and running updates from the Web UI works, but through drush updb doesn't.
It looks to me like the container is loaded without UpdateServiceProvider alteration. I get the 'The service [error]
"new_dependency_test.dependent" has a dependency on a non-existent service "new_dependency_test_with_service.service".' message before any Drupal\Core\Update class is loaded, so I presume drush use some kind of cache OR loads the full container (i.e. bootstrap the full drupal) before starting the update process. I'm not 100% familiar with drush code so I'm just guessing.

Is that just me (drush 8.1.3)?

@gambry the minor issue is fixed.

+++ b/core/lib/Drupal/Core/Update/UpdateServiceProvider.php
@@ -32,6 +32,37 @@ public function alter(ContainerBuilder $container) {
+    if ($removed) {
+      // Recursively remove undefined services if a service had to be removed.
+      $this->removeUndefinedServices($container);
+    }

❓I'm wondering, do we need some kind of recursion protection here? Do we might, for some weird reason, end up in a loop, like when maybe container aliases are involved?

Shouldn't we allow services with missing optional dependencies?
We have some of them in core, for instance core.form_builder, besides using optional arguments can also be a solution developers may apply to temporary have a working container until the new dependent module is enabled. And we don't want to remove those valid services.

If the answer is yes then we should also test we don't remove them.

A couple of fixes to coding standards and namespaces. The UpdatePathNewDependencyTest is still failing because of:

    There was 1 error:

    1)
    Drupal\Tests\system\Functional\Update\UpdatePathNewDependencyTest::testUpdateNewDependency
    Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException:
    The service "new_dependency_test.dependent" has a dependency on a
    non-existent service "new_dependency_test_with_service.service".

    /Volumes/devdisk/dev/sites/drupal8alt.dev/vendor/symfony/dependency-injection/Compiler/CheckExceptionOnInvalidReferenceBehaviorPass.php:58
    /Volumes/devdisk/dev/sites/drupal8alt.dev/vendor/symfony/dependency-injection/Compiler/CheckExceptionOnInvalidReferenceBehaviorPass.php:42
    /Volumes/devdisk/dev/sites/drupal8alt.dev/vendor/symfony/dependency-injection/Compiler/CheckExceptionOnInvalidReferenceBehaviorPass.php:36
    /Volumes/devdisk/dev/sites/drupal8alt.dev/vendor/symfony/dependency-injection/Compiler/Compiler.php:120
    /Volumes/devdisk/dev/sites/drupal8alt.dev/vendor/symfony/dependency-injection/ContainerBuilder.php:573
    /Volumes/devdisk/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/DrupalKernel.php:1307
    /Volumes/devdisk/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/DrupalKernel.php:884
    /Volumes/devdisk/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/DrupalKernel.php:1166
    /Volumes/devdisk/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:179
    /Volumes/devdisk/dev/sites/drupal8alt.dev/core/modules/system/tests/src/Functional/Update/UpdatePathNewDependencyTest.php:39

I think the fail in #38 is happening because we rebuild the container after faking the module install and this picks up the new service. Which is broken with the regular non update container because it is missing

+++ b/core/modules/system/tests/modules/new_dependency_test/new_dependency_test.install
@@ -0,0 +1,14 @@
+  debug('Running the new_dependency_test_update_8001.');

Debug left in which is causing the test to fail. Otherwise the test is looking good.

I think this approach should make it much easier for services to change their dependencies. Nice work.

The patch didn't apply any more since #2901562: Fix 'Squiz.WhiteSpace.SuperfluousWhitespace' coding standard got in. Did a straight rebase on the latest 8.5.x.

+++ b/core/modules/path/src/Form/PathFormBase.php
@@ -192,7 +192,7 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
-      $form_state->setErrorByName('source', t("Either the path '@link_path' is invalid or you do not have access to it.", ['@link_path' => $source]));
+      $form_state->setErrorByName('source', t("The path '@link_path' is either invalid or you do not have access to it.", ['@link_path' => $source]));

Hmm why are we changing these strings? This wasn't present in earlier versions of the patch, it was introduced in #44.

It looks like this is accidentally reverting #2911221: Ungrammatical validation message for paths.

Reverted those accidental changes.

Slightly re-worked some comments and also removed the method and inlined the functionality. The method doesn't really add anything here apart from boilerplate.

+++ b/core/lib/Drupal/Core/Update/UpdateServiceProvider.php
@@ -32,6 +33,32 @@ public function alter(ContainerBuilder $container) {
+          if ($argument instanceof Reference) {
+            if (!$container->has((string) $argument) && $argument->getInvalidBehavior() === ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE) {
+              // If the container does not have the argument and would throw an
+              // exception, remove the service.
+              $container->removeDefinition($key);
+            }
+          }

Is there a reason we don't handle parameters? Maybe we should handle those in a follow up?

I manually tested #49 with update.php, through web, and is fixing the problem. But with drush 9 still fails.

The issue in Drush to use the UpdateKernel for the updatedb command is https://github.com/drush-ops/drush/issues/3193

Drush related issue https://github.com/drush-ops/drush/issues/3193

EDIT: Sorry, I had an old version of the page loaded in browser and I missed @pfrenssen comment.

This looks RTBC to me, but I am not sure about @dawehner's comment:

Is there a reason we don't handle parameters? Maybe we should handle those in a follow up?

The arguments are (AFAIK) either references to other services or strings, and this fix only is concerned about references to non-existing services. What kind of parameters do you mean?

What kind of parameters do you mean?

Well for example '%user.tempstore.expire%'. When I talked with @alexpott about this, he argued that it might be right to do this in a follow up, but well I think its essentially the same problem, we just don't run into it that often with parameters.

+++ b/core/lib/Drupal/Core/Update/UpdateServiceProvider.php
@@ -32,6 +33,32 @@ public function alter(ContainerBuilder $container) {
+    // The kernel cannot be booted if the container such services. This allows

A verb is missing here I think.

The Drush version could be reviewed here https://github.com/drush-ops/drush/pull/3206

Fixed #60. This is a bug, so it's 8.4.x.

@dawehner, something like this?

This breaks database updates when conditions described in "Problem/Motivation" of the issue summary are met. For this reason, this is at least Major (not sure if it's not Critical).

Yeah this is at least major bordering on critical. When this problem occurs you need specialist knowledge to get a working system again. When services are missing during the building of the dependency injection container you cannot bootstrap any more. This means that even the most basic tools that people rely on to restore a broken system (like clearing the cache) suddenly don't work any more.

The parameters part from #64 was wrong because the service referring missing parameter dependencies should be removed. This patch is fixing this, however the test will fail and I cannot figure why. It happens during running updates.

The problem is that the parameters (as parameter bag) are used during compiling the container, in pass ResolveParameterPlaceHoldersPass and... buuum! I have no experience on how to resolve this. Probably adding a new pass before? No idea.

Does option A imply that one needs to run database updates before the site can be set to maintenance mode using drush sset?

@mpp you can avoid that problem by putting the site in maintenance mode at the very start of the process, before the new code is deployed.

Good point, thanks @pfrenssen.

Anybody can help out @claudiu.cristea with an answer for #67? The reported problem can easily break production sites where re-install may not be an option to fix this.

Edited:
Of course nobody updated modules on production with Composer and everybody has backups, but this whole problem makes deployments more problematic because a newly added module (service dependency) have to be deployed and enabled in (production) environment otherwise the site becomes broken. If we could run "drush en missing_dependency" at least in these case that would be a huge help.

Or maybe we would need a solution like registry_rebuild was for Drupal 7 in Drupal 8 too.

Can we do the parameters work in a follow-up, and keeping this issue for the arguments only?
So #62 as latest patch?

I'm concious this is a pretty nasty issue and by fixing at least the arguments bit will be a massive benefit.

+1 for committing #62 and creating a followup for the parameters.

Still found an issue with #62:

+++ b/core/lib/Drupal/Core/Update/UpdateServiceProvider.php
@@ -32,6 +33,33 @@ public function alter(ContainerBuilder $container) {
+            if (!$container->has((string) $argument) && $argument->getInvalidBehavior() === ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE) {
+              // If the container does not have the argument and would throw an
+              // exception, remove the service.
+              $container->removeDefinition($key);

This also seems to remove decorated services using ["@bar.inner"] notation. Which causes other services to give the same error again.

Patch doesn't apply to 8.7.x any more since #3006086: update.php should not process path aliases got in. Here's a reroll of the patch from #63 for 8.7.x.

Spent the day co-working with @justafish who was trying to refactor some services out of a custom module to another custom module and fighting this was extremely painful. I really think we have to do something here. This patch didn't solve the problems because after applying this patch they saw
The service "drush.command.services" has a dependency on a non-existent service "config_split.commands".
During updatedb which I think was because a cache rebuild prior to running updatedb had failed.

You could assume that the cache rebuild prior to running updatedb is a bad thing but this is more akin to the cache free environments core runs it's updates in.

It's very tricky to know what to recommend people to do.

@alexpott, I have a similar case but mine is more complex. The service that I split in the new module (let's say 'service_foo') is called in this hook implementation:

function mymodule_entity_type_alter(...) {
   ...
   \Drupal::service('service_foo');
   ...
}

On update, when the container is build, specifically the date.formatter service, its constructor is doing this:

  public function __construct(EntityManagerInterface $entity_manager, ...) {
    $this->dateFormatStorage = $entity_manager->getStorage('date_format');
    ...
  }

But this leads to building the entity type definitions and consequently the hook_entity_type_alter() implementations are called. Booom!!!

I think building the entity type definitions in a service instantiation is wrong for many reasons. I encounter this problem also with other cases but I cannot recall.

As @bircher alluded to in #10 you can make your services optional, so I just wanted to provide an explicit example in case anyone runs across this looking for a solution now.

Bark service originally provided by labrador:

services:
  labrador.some_handler:
    class: Drupal\labrador\Controller\SomeHandler
    arguments: [ '@labrador.bark' ]

  public function __construct(Bark $bark) {
    $this->bark = $bark;
  }

Moving the Bark service to a new dog module:

services:
  labrador.some_handler:
    class: Drupal\labrador\Controller\SomeHandler
    calls:
      - [setBark, ['@?dog.bark']]

Remove the constructor argument/call and add:

  public function setBark(Bark $bark) {
    $this->bark = $bark;
  }

Just to add some feedback. The patch in number 63 is still giving me the message like this:

The service "drush.command.services" has a dependency on a non-existent service "config_split.commands".

I ended up having to work around it by using the solution posted in #80.

Will you be preparing for 8.7?

I also ran into this while updating blazy from 1->2 at the same time like media_entity to media in core.

The blazy 2 service (@blazy.oembed) depends on a media in core service (@media.oembed.resource_fetcher). But before updb, the media service is not available. The media module will be enabled during the update.

Because of a container rebuild before updb starts, I get "The service "blazy.oembed" has a dependency on a non-existent service "media.oembed.resource_fetcher"."

The patch from #77 allows me to run the update successfully.

Fixed the test and used a more recent dump for a quicker test. It some ways it'd be great to not have to make this a legacy test and not use a db dump at at all. We'd need to re-engineer update path test base to support tests that do a regular install first. I'll open an issue for this as I've hit the same issue in #3063734: Support uninstalling the module providing the update hook in hook_update_N

Since #2975081: UpdatePathTestBase fails to re-initialize the test site (rebuild container, clear caches) after running the database updates we don't need to rebuild the container.

I think we should consider moving forward with this approach because it solves real world problems. Yes it doesn't solve every problem we have but I'm not sure that's possible. For example, the issue that @claudiu.cristea alludes with using a new service in hook_entity_type_alter() is that this will be called in early updates. There's no real way to work around that. So the implement probably needs to be adjusted to do something like \Drupal::hasService() first.

As mentioned in #85 this allows us to run the blazy update at the same time like the media_entity -> media update. So I am marking this as RTBC.

I am also raising the priority to critical because without this patch we have no chance to do the update. I would also vote for a backport to 8.7.x.

https://www.drupal.org/node/45111#critical-bug

I agree that it would be good to have this in, even though it does not fix all edge cases. The main problem that most people encounter is fixed and it can be very puzzling to figure this out. We have been using the various versions of this patch in production already since 2017.

I reviewed the 2 recent interdiffs and they look good to me. RTBC +1.

For a moment I was thinking we might end up hiding errors here, but this only happens during updates so if someone really has defined a broken service definition it'll still be reported.

Can't think of anything else we can do here, and agreed we only need to improve things here rather than fix every possible scenario.

Committed 8f430db and pushed to 8.8.x. Thanks!

Talked shortly with @catch in slack and he sees a chance to commit this to 8.7.x after the next patch release.

Thank you everyone who worked so hard on this; I have run into this issue quite often on long-standing projects and it's nice to not need to add awkward checks to event subscribers and similar things just to work around this item.

Even though this doesn't create an API change, would it be helpful to have a Change Record in order to describe to developers how this works, and if there is any action they need to take in update hooks in order to take advantage of this? From a read of the commit I'm not 100% sure on what the proper workflow is.

I'd be happy to help write one if I can get confirmation on the items above. Thanks!

@bradjones1 there's nothing for a developer to do. If the added service is not use in update hooks and has missing dependencies at the beginning but not at the end of the updates then everything will happily work. There are a number of situations this patch does not fix though so writing a change record is going to be tricky because there's nothing for a developer to change and covering all the edge cases is hard.

Unfortunately though we need to roll this back as this breaks service decoration. It'll remove a service which is using the .inner service as an argument see https://symfony.com/doc/current/service_container/service_decoration.html for how service decoration works and how it creates a pseudo service of .inner.

+++ b/core/lib/Drupal/Core/Update/UpdateServiceProvider.php
@@ -52,6 +53,33 @@ public function alter(ContainerBuilder $container) {
+            if (!$container->has((string) $argument) && $argument->getInvalidBehavior() === ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE) {
+              // If the container does not have the argument and would throw an
+              // exception, remove the service.
+              $container->removeDefinition($key);
+            }

This needs to check if the definition is decorating a service and do special logic.

Reverted, back to 8.8.x

Here's a patch that fixes the issue with using decorated services during update and adds a test.

This patch addresses #104 and it also removes the usage of the word decorated when it is not to do with container service decoration as that is very confusing.

In testing this patch with the Thunder 2 to Thunder 3 update path @chr.fritsch discovered that this causes problems for drush services - which are declared in drush.services.yml. This is because these are added to the container via a compiler pass. Which is exactly what we should be using here. There's a special stage for compiler passes that remove services - these are guaranteed to come at the end - so we can use that. This also means we can lose our special code for decorated services because we're now coming after \Symfony\Component\DependencyInjection\Compiler\DecoratorServicePass().

Also we can use Symfony's RepeatedPass code to keep the logic simpler.

Note I think the latest patch fixes #78 too.

Here's how to test #106.

1. Install Drupal 8.6.x
2. Install config_split and config_filter (latest dev versions)
3. Install Blazy 8.x-1.x
4. Update code to Drupal 8.8.x and Blazy 8.x-2.x
5. Run drush updatedb - it'll fail
6. Apply patch
7. It'll now work

Ho hum RepeatedPass is deprecated in Symfony 4.2 so let's not use that...

Removed repeated pass and put more testing in place to ensure we triggered multiple runs through the while to remove services. Also added logging to the container compilation because we can.

We can make further optimisations because of \Symfony\Component\DependencyInjection\Compiler\ResolveReferencesToAliasesPass which we come after.

Amazing work! This is one of the worst bug I always fear when upgrading contrib.

Two minor, which can be fixed on commit. So leaving RTBC.

+++ b/core/lib/Drupal/Core/Update/UpdateCompilerPass.php
@@ -0,0 +1,62 @@
+    // We only need to check aliases is we've removed a service because

Something weird with this sentence? is -> if?

+++ b/core/modules/system/tests/src/Functional/Update/UpdatePathNewDependencyTest.php
@@ -0,0 +1,91 @@
+    // dependency is still available while the ones that fail are not.
+
+    try {

Unneeded blank line?

@gambry thanks for the review - yep those comments need work. I've re-read all the docs in \Drupal\Core\Update\UpdateCompilerPass and tried to improve it.

Committed/pushed to 8.8.x and cherry-picked to 8.7.x, thanks!

Don't you think this issue requires a mention in next release notes, or even a change record?
Although there are not API changes itself, we've been advocating for the usage of setter injection or ignoring missing dependencies, which now is not required anymore.

I believe quite few contrib and custom modules would love to know the problem has been fixed and they can now stop doing it?

Having had to find a workaround for this quite recently, a change record sounds like a good idea to me! Also, thanks to everybody who worked on this!

#118 +1
#119 +1

Fixed a typo and published the CR.

I was going to add a bit of formatting (see end of this comment), but I don't get what this paragraph means:

This allows installed modules to be refactored to depend on yet uninstalled modules and have the new modules services injected in the refactored services. The existing module must install the new module in an update hook because the normal service container will throw an exception when there are unmet dependencies.

What's "this"? UpdateKernel itself, or the changes?

I think our changes avoid a service refactoring, as long as the module install its dependencies on the hook_update()?

--- formatted CR ---
Summary
The UpdateKernel which is used by update.php to run the update hooks now removes services and aliases with unmet dependencies from its dependency injection container.

Before
Before this change module developers had to use setter injection or ignoring missing dependencies and guarding their services for those cases because update hooks could not be run due to the container throwing an exception.

After
The changes introduced with #2863986: Allow updating modules with new service dependencies allow installed modules to be refactored to depend on yet uninstalled modules and have the new modules services injected in the refactored services services to depend on yet uninstalled module services as long as the existing module install the new module in an update hook, and that's because the normal service container will throw an exception when there are unmet dependencies.

Please note those services with unmet dependencies will not be available during update hooks.

Here is the patch for Drupal 8.6 based on the commit #117.

We've got an advanced case over in #3085751: Setter injection arguments are not checked for unmet dependencies - fortunately this issue gives us all the plumbing to fix and test - so yay.

Also logging a similar follow-up to this here: #3110242: Allow updating modules with services that decorate new service dependencies.