From 92812c76d3715e21d76df51f1a178ab6903ffa4e Mon Sep 17 00:00:00 2001 From: catch Date: Fri, 17 Jul 2020 09:02:25 +0100 Subject: [PATCH] Issue #3085751 by alexpott, Deepak Goyal, rpayanm, longwave, catch, volkerk, kristiaanvandeneynde, dww: [backport] Setter injection arguments are not checked for unmet dependencies (cherry picked from commit 2a71dd80fe94e30ebff5d76aff6f2a174c793fd8) --- .../Drupal/Core/Update/UpdateCompilerPass.php | 46 ++++++++++++++++--- .../new_dependency_test.install | 1 + .../new_dependency_test.services.yml | 4 ++ .../src/SetterInjection.php | 39 ++++++++++++++++ .../UpdatePathNewDependencyTest.php | 3 ++ 5 files changed, 87 insertions(+), 6 deletions(-) create mode 100644 core/modules/system/tests/modules/new_dependency_test/src/SetterInjection.php diff --git a/core/lib/Drupal/Core/Update/UpdateCompilerPass.php b/core/lib/Drupal/Core/Update/UpdateCompilerPass.php index 6ac05886d0..73cef024ec 100644 --- a/core/lib/Drupal/Core/Update/UpdateCompilerPass.php +++ b/core/lib/Drupal/Core/Update/UpdateCompilerPass.php @@ -27,16 +27,28 @@ public function process(ContainerBuilder $container) { do { $has_changed = FALSE; foreach ($container->getDefinitions() as $key => $definition) { + // Ensure all the definition's arguments are valid. foreach ($definition->getArguments() as $argument) { - if ($argument instanceof Reference) { - $argument_id = (string) $argument; - if (!$container->has($argument_id) && $argument->getInvalidBehavior() === ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE) { - // If the container does not have the argument and would throw an - // exception, remove the service. + if ($this->isArgumentMissingService($argument, $container)) { + $container->removeDefinition($key); + $container->log($this, sprintf('Removed service "%s"; reason: depends on non-existent service "%s".', $key, (string) $argument)); + $has_changed = TRUE; + $process_aliases = TRUE; + // Process the next definition. + continue 2; + } + } + + // Ensure all the method call arguments are valid. + foreach ($definition->getMethodCalls() as $call) { + foreach ($call[1] as $argument) { + if ($this->isArgumentMissingService($argument, $container)) { $container->removeDefinition($key); - $container->log($this, sprintf('Removed service "%s"; reason: depends on non-existent service "%s".', $key, $argument_id)); + $container->log($this, sprintf('Removed service "%s"; reason: method call "%s" depends on non-existent service "%s".', $key, $call[0], (string) $argument)); $has_changed = TRUE; $process_aliases = TRUE; + // Process the next definition. + continue 3; } } } @@ -58,4 +70,26 @@ public function process(ContainerBuilder $container) { } } + /** + * Checks if a reference argument is to a missing service. + * + * @param mixed $argument + * The argument to check. + * @param \Symfony\Component\DependencyInjection\ContainerBuilder $container + * The container. + * + * @return bool + * TRUE if the argument is a reference to a service that is missing from the + * container and the reference is required, FALSE if not. + */ + private function isArgumentMissingService($argument, ContainerBuilder $container) { + if ($argument instanceof Reference) { + $argument_id = (string) $argument; + if (!$container->has($argument_id) && $argument->getInvalidBehavior() === ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE) { + return TRUE; + } + } + return FALSE; + } + } diff --git a/core/modules/system/tests/modules/new_dependency_test/new_dependency_test.install b/core/modules/system/tests/modules/new_dependency_test/new_dependency_test.install index 2ab3e6b498..552d6912c8 100644 --- a/core/modules/system/tests/modules/new_dependency_test/new_dependency_test.install +++ b/core/modules/system/tests/modules/new_dependency_test/new_dependency_test.install @@ -17,6 +17,7 @@ function new_dependency_test_update_8001() { 'new_dependency_test.alias_dependency', 'new_dependency_test.alias2', 'new_dependency_test.alias_dependency2', + 'new_dependency_test.setter_injection', ]; // Gather the state of the services prior to installing the diff --git a/core/modules/system/tests/modules/new_dependency_test/new_dependency_test.services.yml b/core/modules/system/tests/modules/new_dependency_test/new_dependency_test.services.yml index a3283daea8..1f1c4ca29a 100644 --- a/core/modules/system/tests/modules/new_dependency_test/new_dependency_test.services.yml +++ b/core/modules/system/tests/modules/new_dependency_test/new_dependency_test.services.yml @@ -34,3 +34,7 @@ services: decorates: new_dependency_test.another_service_two decoration_inner_name: new_dependency_test.foo arguments: ['@new_dependency_test.foo'] + new_dependency_test.setter_injection: + class: Drupal\new_dependency_test\SetterInjection + calls: + - [setter, ['@new_dependency_test_with_service.service']] diff --git a/core/modules/system/tests/modules/new_dependency_test/src/SetterInjection.php b/core/modules/system/tests/modules/new_dependency_test/src/SetterInjection.php new file mode 100644 index 0000000000..d74ee49de2 --- /dev/null +++ b/core/modules/system/tests/modules/new_dependency_test/src/SetterInjection.php @@ -0,0 +1,39 @@ +service = $service; + } + + /** + * Get the simple greeting from the service. + * + * @return string + * The greeting. + */ + public function greet() { + return $this->service->greet(); + } + +} diff --git a/core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathNewDependencyTest.php b/core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathNewDependencyTest.php index 6830c3b7f5..e8eab8372b 100644 --- a/core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathNewDependencyTest.php +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathNewDependencyTest.php @@ -55,6 +55,7 @@ public function testUpdateNewDependency() { $this->assertEquals('Hello', $this->container->get('new_dependency_test.alias')->greet()); $this->assertEquals('Hello World', $this->container->get('new_dependency_test.hard_dependency')->greet()); $this->assertEquals('Hello World', $this->container->get('new_dependency_test.optional_dependency')->greet()); + $this->assertEquals('Hello', $this->container->get('new_dependency_test.setter_injection')->greet()); // Tests that existing decorated services work as expected during update. $this->assertTrue(\Drupal::state()->get('new_dependency_test_update_8001.decorated_service'), 'The new_dependency_test.another_service service is decorated'); @@ -70,6 +71,7 @@ public function testUpdateNewDependency() { 'new_dependency_test.alias_dependency' => FALSE, 'new_dependency_test.alias2' => FALSE, 'new_dependency_test.alias_dependency2' => FALSE, + 'new_dependency_test.setter_injection' => FALSE, ], $before_install); $after_install = \Drupal::state()->get('new_dependency_test_update_8001.has_after_install', []); @@ -81,6 +83,7 @@ public function testUpdateNewDependency() { 'new_dependency_test.alias_dependency' => TRUE, 'new_dependency_test.alias2' => TRUE, 'new_dependency_test.alias_dependency2' => TRUE, + 'new_dependency_test.setter_injection' => TRUE, ], $after_install); } -- GitLab