Problem/Motivation

The symfony service container allows to put a "bind" key in https://symfony.com/doc/current/service_container.html#binding-arguments...

Currently this is not enabled in Drupal.

It would solve a similar purpose as the global aliases described in #3367416: [policy] Service autowiring - decide approach for multiple service implementations based on the same class and different arguments.
But instead of a global mapping, it binds only the services within a single *.services.yml file.
This way, a module can bind its own instance(s) of logger or other services, without side effects on core or other modules.

Steps to reproduce

In a *.container.yml file, do this:

services:
    _defaults:
        bind:
            # pass this service for any LoggerInterface type-hint for any
            # service that's defined in this file
            Psr\Log\LoggerInterface: '@logger.channel.mymodule'
    logger.channel.mymodule:
        parent: logger.channel_base
        arguments: ['mymodule']

(to be continued)

Proposed resolution

Enable the bind key.
Write tests.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#18 3375968-nr-bot.txt90 bytesneeds-review-queue-bot

Issue fork drupal-3375968

Command icon Show commands

Start 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:

About issue forks

Comments

donquixote created an issue. See original summary.

donquixote opened merge request !4428

donquixote’s picture

donquixote commented

For now only support 'bind' in '_defaults' section.

By looking at the code in symfony, symfony also supports 'bind' in a specific service definition.
The current MR does not support this.
Also, I don't see documentation for it in symfony.

donquixote’s picture

donquixote commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

I thoroughly enjoy reading these kind of tickets.

The test coverage looks like it covers everything.

Should there be follow up tickets (maybe a META) to update existing services to bind at least a logger to each module?

Putting into the committer bucket.

donquixote’s picture

donquixote commented

Btw, I notice the phpcs:ignoreFile, and the 4 spaces indent in the core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php.

I also notice that I have a comment without a dot, which would normally trip up the code style check. But here it goes unnoticed.
(That comment line is copied 1:1 from symfony, and it explains why they do unserialize(serialize()) deep clone)
So, we need some human eyes no the code style!

---

By looking at the code in symfony, symfony also supports 'bind' in a specific service definition.
The current MR does not support this.

The reason was that in symfony, the code to support specific service 'bind' does something with a $this-isLoadingInstanceof, which does not exist in the Drupal YamlFileLoader, and which I don't fully understand.
This also shows that the requirement to "document everything" in Drupal is not always bad - if the documentation actually adds something.

donquixote’s picture

donquixote commented

Actually we can ignore ->isLoadingInstanceof for now.
It only becomes relevant if we support a _instanceof key in *.services.yml.
At some point we might do this, but not for now.

donquixote’s picture

donquixote commented

There is also the $trackBindings parameter, which seems to be always true, but I am not sure about it.

donquixote’s picture

donquixote commented

Should there be follow up tickets (maybe a META) to update existing services to bind at least a logger to each module?

Actually, what we should do, before this is merged:
Create a proof-of-concept branch in this issue, where we convert a bunch of things to use "bind", to detect if there are unpleasant surprises waiting for us.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Great work!

+1 to testing this further by converting some core modules to use it. It is hard to pick out all the parts that we need from the Symfony parser code because they have so many features we are missing, some of which interact with each other, and there are very few comments,

donquixote’s picture

donquixote commented

Also testing bind + autowire.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Reviewed & tested by the community » Needs work

Last test needs fix

donquixote opened merge request !4443

donquixote’s picture

donquixote commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Come back to this a few times with fresh eyes.

To me (and my novice skill in this area) the changes look good.
Binding the logger and the fact it doesn't break the tests for the submodules seems like its working.

Wonder what the committers think?

quietone’s picture

quietone commented

I triaged this RTBC issue sometime in the last weeks but didn't leave a comment.

There are no unanswered questions here but I do have one. From #10

+1 to testing this further by converting some core modules to use it. It is hard to pick out all the parts that we need from the Symfony parser code because they have so many features we are missing, some of which interact with each other, and there are very few comments,

Are all core modules converted here? Is there followup work to do?

This can stay at RTBC.

catch’s picture

catch
he/him
Primary language English
commented

I think it's OK if we just convert a couple of examples here without doing everything or even a follow-up to do everything. We will eventually have an issue to autowire all core services, but that might end up depending on other issues like this.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3375968-nr-bot.txt90 bytes

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 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.

acbramley’s picture

acbramley commented

I'm interested in helping to move this issue along. Given #17, should we just get the PoC MR green? The few examples there cover a decent number of scenarios.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.