Setter injection arguments are not checked for unmet dependencies

Interesting so this is happening because fb_instant_articles has added a new dependency on the serialization module which declares a service that's now used via setter injection. I wonder why fb_instant_articles is using setter injection - anyhow I guess we need to look through the methods as well and see if they are using any services which cannot be meet. As a quickfix fb_instant_articles could allow a null to be passed in the setter. Ie.

    calls:
     - [setSerializer, ['@?serializer']]
     - [setLogger, ['@logger.channel.fbia']]
     - [setIaNormalizer, ['@serializer.fb_instant_articles.fbia.content_entity']]

public function setSerializer(NormalizerInterface $serializer) { will need a change too.

Another question is why is it necessary to construct this service during an update.

Now we have the UpdateCompilerPass this is not too tricky to deal with fortunately.

@volkerk you're totally right - great catch.

I think this is a major bug because without this you potentially can't upgrade a contrib module which in theory could prevent you from getting a security upgrade.

Logic looks fine but this needs a re-roll.

#11 was missing a file.

Nice :)

Looks good to me. Wish we could do the same for 'factory:' also :)

Actually, here we go: #3150512: [PP-1]: Factory arguments are not checked for unmet dependencies

Mostly this looks great, and *much* needed. Thanks! Only found a few very minor documentation cleanups. Could be a follow-up, but probably easier to just fix them quickly here. Happy to re-RTBC with a new patch.

1. +++ b/core/lib/Drupal/Core/Update/UpdateCompilerPass.php
   @@ -27,16 +27,28 @@ public function process(ContainerBuilder $container) {
   +            // Process the next definition.
   +            continue 2;
   ...
   +              // Process the next definition.
   +              continue 3;
   

   These comments are great, thanks! Really helpful to orient readers on why 2 vs. 3 in the continue statements.

2. +++ b/core/lib/Drupal/Core/Update/UpdateCompilerPass.php
   @@ -58,4 +70,26 @@ public function process(ContainerBuilder $container) {
   +   * Checks is if a reference argument is to a missing service.
   

   s/is if/if/

3. +++ b/core/lib/Drupal/Core/Update/UpdateCompilerPass.php
   @@ -58,4 +70,26 @@ public function process(ContainerBuilder $container) {
   +   * @param $argument
   

   Is @param mixed $argument more accurate?

4. +++ b/core/modules/system/tests/modules/new_dependency_test/src/SetterInjection.php
   @@ -0,0 +1,39 @@
   +   * InjectedService constructor.
   

   s/InjectedService/SetterInjection/

Hi @dww
Updated patch as you suggested please review.

LGTM

Yup, this is ready.

Thanks!
-Derek

Committed c70642a and pushed to 9.1.x. Thanks!

Moving to 9.0.x for backport consideration.

Been hesitating on the backport here because of the new protected method. Discussed with @xjm and she suggested inlining the logic for backport, which makes sense to me.

There is not really a use-case for extending this compiler pass. If you did you'd have to somehow unregistered this one - which would be very hard to do. Because it's hardcoded in \Drupal\Core\Update\UpdateServiceProvider which is hardcoded in \Drupal\Core\Update\UpdateKernel::discoverServiceProviders. I think that changing the method to be inline or even private creates an unnecessary variance which makes any future bugfixes harder. In an ideal world all of Drupal's implementations of CompilerPassInterface would be marked with @internal but given how they are used - ie. all statically hardcoded by design - that's strongly implied anyway.

Re why this should be backported: if you add a new service and change an existing service to use setter injection to get this new service in a dependent module it's just not possible for Drupal to update. This bug was discovered through a real-world use-case affected Drupal 8.9.x

So here's a way to have 0 BC concerns, reflect the static nature of compiler passes and have no variance between 9.1.x and earlier branches... we can make the new method private everywhere.

The Drupal 9.1.x patch is the interdiff.

Private method is a good idea to me, back to RTBC.

Committed/pushed the follow-up to 9.1.x and the new version to 9.0.x and 8.9.x, thanks!