Inject services into LoggerChannelFactory

Code changes are minimal and make sense, so does the reasoning in the IS, green tests => RTBC

Let's write a change record in case someone is overriding this for some reason and adding their own constructor.

Added BC, because we should be able to inject services if the container is available. As per deprecation policy we can link the message to the issue, not sure this is worth it's own CR, so linked it here.

Well....

The new BC triggers deprecation messages on basically every test using a container...

Yeah, that didn't work, there is a container available but it's not the right one. So a slightly uglier approach - if we explicitly pass NULL then that means we are in the installer, otherwise trigger the deprecation and BC layer.

https://3v4l.org/pcU1u implies this should be OK.

Besides adding some NULL, NULL to new LoggerChannelFactory() in \Drupal\Tests\media\Unit\ProviderRepositoryTest and \Drupal\Tests\Core\Logger\LoggerChannelFactoryTest, I think you're onto a winner.

Fixed LoggerChannelFactoryTest as suggested.

Refactored ProviderRepositoryTest to inject a mock LoggerChannelFactory instead of a real object.

PHPCS hates you...;)
#TheStruggleIsReal

Refactored ProviderRepositoryTest to inject a mock LoggerChannelFactory instead of a real object.

Is this because we want to mock as much as possible in unit tests?

And in LoggerChannelFactoryTest we can't mock LoggerChannelFactory since it's the SUT?

Fixed PHPCS.

Re #12, yes, in unit tests it is better to mock things that you aren't specifically testing. Here, we don't actually care what LoggerChannelFactory does internally, except that it needs to provide a LoggerChannel for the ProviderRepository to use - so we inject a mock that only does that. But in LoggerChannelFactoryTest, we actually want to test the functionality of LoggerChannelFactory itself, so we want a real object in order to prove that it behaves correctly.

Which made me think that I could also improve LoggerChannelFactoryTest: we might as well inject mock versions of the correct arguments, so I made that change.

Nice, like the added mocked arguments.

All my (endless) moaning has been addressed, green tests => RTBC.

Also realised we can likely remove the NULL, NULL special case once 📌 Remove dependency of "file_system" service on "logger" Needs work lands.

Merge conflicts on the MR.

Let's postpone this on 📌 Remove dependency of "file_system" service on "logger" Needs work as that makes this easier anyway.

Rebased after 📌 Fix install container definition of StreamWrapperManager Needs review

Left 1 question on the MR.

Following 📌 Fix install container definition of StreamWrapperManager Needs review I think we can simplify this.

Unsure of the value of a change record here. The likelihood that someone is overriding the entire logger factory seems low to me. But for completeness it won't take long to write one.

Removed the condition, ++ to not add CR as we do all other places

As the request stack and current user are now always available, we can always inject them into the created LoggerChannel.

I bet we should provide BC to allow old container to work for upgrades at least

The constructor will ensure the new properties are always set, so $this->container is no longer needed at all?

Good point - fixed

Injection seems fine.

Seems there's still open discussion about when to use a CR vs linking to a ticket. Not sure if that should hold up things so will go ahead and mark.

Question on the MR.

Responded to the feedback.

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

Rebased, responded to feedback.

Given there were no changes here, being bold and marking RTBC to get this back with other committers for re-review.

What's the plan for things like:

• https://git.drupalcode.org/project/toggle_channels/-/blob/8.x-1.x/src/Lo...
• https://git.drupalcode.org/project/tmgmt_smartling/-/blob/8.x-5.x/module...
• https://git.drupalcode.org/project/test_helpers/-/blob/1.4.x/src/Stub/Lo...

I think all of these will be broken after this change. At the very least we need a change record because I think we could allow this under the constructors are not API. But I think we need to consider BC here and what to do.

I think we can add BC for all of those cases, it might be a bit awkward but looks doable.

Added BC and deprecations for calling get() without the constructor being called, setContainer() being called, and $this->container being accessed, which I think covers all the cases in #34.

Out of time for now, still needs a change record.

Added few comments but the BC is the main concerns

@andypost good questions/suggestions, thank you - responded in MR

Thank you! Then it's ready, follow-up to remove bc can be filled later

The bc layer is tricky but we get to remove it in the 11.x branch soon, which is good. Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

After committing this, I realised it was tagged for a change record and didn't have one. I'm not really convinced we should add change records for constructor deprecations, we're already going above and beyond providing bc for code that supposed to be @internal, but since we know in this case that contrib will be affected, went ahead and added https://www.drupal.org/node/3432835 →

catch → closed merge request !6280

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