Add autoconfigure for module loggers

The main technical challenge would be having different logger instances (channels) autowired per module.

For those new to autoconfigure, we have an instance in core as of 📌 Enable service autoconfiguration for core event subscribers Fixed

Change record: https://www.drupal.org/node/3357408 →

kim.pepper → made their first commit to this issue’s fork.

Had a quick look at this, and it doesn't look like we know which module the services.yml is being loaded from. By the time we get to CoreServiceProvider we have a fully merged container definition.

This would make it hard to auto configure file.services.yml with a logger service id logger.channel.file for example.

One option is to loop through the service definitions and try to guess a prefix?

I was looking at \Drupal\Core\CoreServiceProvider::register() and adding something like:

$container->registerForAutoconfiguration(LoggerAwareInterface::class)
    ->addMethodCall('setLogger', [new Reference('logger.channel.default')]);

YamlFileLoader::parseDefinitions() appears to add a _provider tag to each service with the name of the module.

See also #3295542-10: Use logger channel services consistently, instead of the factory object → where I suggested a similar idea.

Nice!

Having a look at this.

Tried an approach of first adding a logger_aware tag to any service with a class implementing \Psr\Log\LoggerAwareInterface by using +

$container->registerForAutoconfiguration(LoggerAwareInterface::class)
      ->addTag('logger_aware');

Then a \Drupal\Core\DependencyInjection\Compiler\LoggerAwarePass that looks for those tagged services.

Not finding my test service at the moment, so I thought I'd push up what I have to see if others have ideas?

Needed to add:

services:
  _defaults:
    autoconfigure: true

to logger_aware_test.services.yml.

Added a change record.

Manually tested with services I was playing with before creating this issue and (after applying the other patch to allow autoconfigure) it's working really well. Nice one!

The CR doesn't mention the 'logger_aware' tag, but the code looks like it's still using it.

I don't think there are any big issues but just to make sure we considered it: are there BC concerns here where services might have previously implemented LoggerAwareInterface and now we have added behaviour to that? Should we also check that the setLogger() method call isn't already present, just in case?

Agreed we should probably he checking for existing calls, is it also possible existing class members called $logger could be overwritten? I suppose the interface/trait covers us there.

We should also add a before and after to the CR showing how you no longer need to inject the logger.

Should we also check that the setLogger() method call isn't already present, just in case?

Sorry, not sure I follow... We are only checking services that implement LoggerAwareInterface. The setLogger() method will always be there if they implement LoggerAwareInterface.

Updated CR

Ah I think I follow now.

Should we also check that the setLogger() method call isn't already present, just in case?

Sorry, not sure I follow... We are only checking services that implement LoggerAwareInterface. The setLogger() method will always be there if they implement LoggerAwareInterface.

We just need to check if the definition already has a setLogger method call defined.

I wonder if we should also detect LoggerInterface being used in service constructor arguments (as opposed to a setter method) and automatically create and inject a logger? Or defer that to a followup?

Thinking again we should only do #23 if autowire: true is set which probably means we have to wait for 📌 Use autowiring for core modules and services Needs work

This looks good to me as a self contained issue. I don't think we need to explicitly test that exception, nothing else to do here so this is RTBC.

One minor issue with PHPCS

Tests green ✅

Committed to 11.x and backported to 10.2.x

Published CR

Automatically closed - issue fixed for 2 weeks with no activity.

kim.pepper → closed merge request !5060