Create LoggerChannelTrait

Added LoggerChannelTrait, used it in ControllerBase and FormBase.

ControllerBase had getLogger(). FormBase had logger(). getLogger() wins for being more descriptive, so LoggerChannelTrait::getLogger() exists.

FormBase::logger() now just calls LoggerChannelTrait::getLogger(), with a note in docs about how it's a compatibility shim.

If this trait is desired by maintainers, we can go through and change calls to logger() with getLogger() and remove FormBase::logger() in a subsequent patch.

Ewps.

Added return. :-) Also added a @todo.

OK, I took out the @todo because I think the 'don't use this' is enough, and because this issue is really yak shaving for #2729625: Inject services into BookAdminEditForm and ultimately #2729597: [meta] Replace \Drupal with injected services where appropriate in core. We don't want to have to re-implement logger() every time we decide FormBase is bad. Which it is. :-) #2674508: Improve docs for how to properly add container injection into a class that extends FormBase

Nice catch on public vs. protected.

Addressed here.

The two implementations this replaces get the logger from the container directly. Can we also do that here, and just enforce that the classes that use it are container aware?

Ab.
So.
Lutely.

I was following the example of the other service traits for consistency.

I would be more than happy to open follow-ups to modify the other traits in the same way. :-)

Used assert() to enforce the existence of the service.

Since this was me learning assert(), there's also a test. :-)

OK, so that list of failed tests is a nice list of action items for #2733703: [plan] Service traits should require IoC injection which I just created.

Converting back to \Drupal for now, and adding a @todo about #2733703: [plan] Service traits should require IoC injection

The plan might be to use \Drupal and then switch over to requiring the injected service once the classes which use the trait have been converted to IoC.

Maintainers can of course have a differing idea. :-)

Yah, I didn't get what you meant, but the reason is that there's no $this->container, or at least there shouldn't be. :-)

The trait wraps a service that's already been injected, and if it's not there, \Drupal is the backup. The problem with that is the service dependency becomes a magic one.

So we might solve that by requiring a container, but then we lose the many benefits of inversion of control, mainly enabling SOLID in more places.

So if we want this trait to be strict about it's users having already injected the dependency, then we'd assert on the specific dependency, rather than the container. In this case, we don't care where the LoggerChannelFactoryInterface came from, only that we have one.

That means our trait only has one dependency: LoggerChannelFactoryInterface.

That's better than having one explicit but non-specific dependency (the container), plus a magic one.

Reworked the patch from #19 because I forgot to remove the assertion test.

Erp... Wrong interdiff.

This should only be used in application-level code that's already container aware though - as the two examples converting to it are.

You might want to research that.

There's no such thing as ContainerAwareInterface in Drupal, which is a good thing.

FormBase implements ContainerInjectionInterface, and doesn't store the container and doesn't give you a way to access the container.

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Form%21Fo...

ControllerBase gives you an accessor for \Drupal which a) makes no sense because it's marked private and why don't those other methods just call \Drupal? And b) is not a part of any interface.

So it's all magic voodoo. :-)

I am willing to do the work to untangle all this, as I have been doing.

But in order to start, I need a trait instead of re-implementing logger() all the time.

Yes indeedy ContainerAwareInterface: 21 files implement it. Then we have 22 files with 'use ContainerAwareTrait'. Take a look at EntityTypeManager::createHandlerInstance(), where we check for the existence of trait methods. That's the kind of code smell I'm talking about, and why we should do things like assert that the dependency is already discovered as per #17.

Anyway...

So what we have in patch #21 is similar to the other service traits, in that if the user didn't initialize the service (bad user, bad!), the trait will grab it from \Drupal. As the todo points out, this is a halfway measure and we might potentially replace that with an assertion or with something else entirely.

We also use the trait in two base classes, ControllerBase and FormBase, which seems like a good place to start. We re-implement FormBase::logger() to use the trait's getLogger().

Once this is in we can then re-roll #2729625: Inject services into BookAdminEditForm for the log service, and move forward on something like #2729597: [meta] Replace \Drupal with injected services where appropriate in core without re-implementing logger discovery in every class that uses it.

The patch in #21 contains a @todo pointing to this issue: #2733703: [plan] Service traits should require IoC injection which is where we really should have had the conversation starting in #20. :-)

Plus contrib can now use the logger trait. :-)

Anyone care to review?

@jibran: It's a different thing. We want to emulate traits like StringTranslationTrait and LinkGeneratorTrait. These do not supply interfaces.

ContainerAwareTrait exists so you have a default way of implementing ContainerAwareInterface, so that you can store the container. We don't want to supply a default method for injecting specific dependencies (in this case, logger factory) for controllers or forms. We only want a way to avoid re-implementing boilerplate for *using* those dependencies.

Clicking on that cgit link... http://cgit.drupalcode.org/drupal/commit/?id=f7b235c

"Bad object id: f7b235c"

Not pushed yet?

Awesome. Thanks. :-)