Service collector won't collect child services with no class definition, due to \Drupal\Core\DependencyInjection\Compiler\TaggedHandlersPass::processServiceCollectorPass only check matching interface for definition itself, but not checking parent's class.
// Validate the interface.
$handler = $container->getDefinition($id);
if (!is_subclass_of($handler->getClass(), $interface)) {
throw new LogicException("Service '$id' for consumer '$consumer_id' does not implement $interface.");
}
Create child service with tag:
my_module.root_handler:
class: Drupal\MyModule\CustomTranslator
abstract: true
my_module.parent_handler:
parent: my_module.root_handler
my_module.child_handler:
parent: my_module.parent_handler
tag:
- { name: string_translator }
The container will throw the following exception when compiling:
Symfony\Component\DependencyInjection\Exception\LogicException: Service 'my_module.child_handler' for consumer 'string_translation' does not implement Drupal\Core\StringTranslation\Translator\TranslatorInterface.
even though Drupal\MyModule\CustomTranslator implement that interface.
Make \Drupal\Core\DependencyInjection\Compiler\TaggedHandlersPass::processServiceCollectorPass check parent's class for matching interface for child service.
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | 3309334-nr-bot.txt | 1.98 KB | needs-review-queue-bot |
| #11 | 3309334-nr-bot.txt | 1.98 KB | needs-review-queue-bot |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !2756
Comments
Comment #3
cilefen commentedComment #6
el7cosmosComment #7
smustgrave commentedSee that 2 tests were added so would expect both to fail but testProcessChildDefinitionParentInterfaceMismatch passes the test-only feature.
Comment #8
el7cosmosthe
::testProcessChildDefinitionParentInterfaceMismatchmethod is supposed to pass even without the changesComment #9
smustgrave commentedThen why add it? If it's not testing anything new? Expanding test coverage may be considered out of scope.
Comment #10
el7cosmosI just removed additional test
Comment #11
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.
Comment #12
smustgrave commentedFalse positive
Comment #13
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.
Comment #14
el7cosmosComment #15
smustgrave commentedLeft some comments on the MR.
Comment #16
el7cosmosComment #17
smustgrave commentedBelieve all feedback has been addressed
Ran test-only job here https://git.drupalcode.org/issue/drupal-3309334/-/jobs/5610629 which resulted in
Comment #20
catchCommitted/pushed to 11.x - I think we need to allow contrib modules to have the same definition between 11.x and 10.x so also cherry-picked to 10.6.x, thanks!