In #3397522: Fork Symfony's ContainerAwareTrait and ContainerAwareInterface into core we are trying to reduce the use of ContainerAwareTrait as Symfony has deprecated it.
LoggerChannelFactory retrieves the request stack and current user services from the container. There seems to be no reason for this, they could just be injected in the constructor in the normal way.
Inject the services directly.
| Comment | File | Size | Author |
|---|---|---|---|
| #31 | 3416354-nr-bot.txt | 90 bytes | 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 !6280
Comments
Comment #3
longwaveComment #4
longwaveComment #5
spokjeCode changes are minimal and make sense, so does the reasoning in the IS, green tests => RTBC
Comment #6
longwaveLet's write a change record in case someone is overriding this for some reason and adding their own constructor. However as it's possible to actually inject NULL (in the case of the installer) I'm not sure if or how we can provide BC.
Comment #7
longwaveAdded 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.
Comment #8
spokjeWell....
The new BC triggers deprecation messages on basically every test using a container...
Comment #9
longwaveYeah, 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.
Comment #10
spokjeBesides adding some
NULL, NULLtonew LoggerChannelFactory()in\Drupal\Tests\media\Unit\ProviderRepositoryTestand\Drupal\Tests\Core\Logger\LoggerChannelFactoryTest, I think you're onto a winner.Comment #11
longwaveFixed LoggerChannelFactoryTest as suggested.
Refactored ProviderRepositoryTest to inject a mock LoggerChannelFactory instead of a real object.
Comment #12
spokjePHPCS hates you...;)
#TheStruggleIsReal
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?
Comment #13
longwaveFixed 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.
Comment #14
spokjeNice, like the added mocked arguments.
All my (endless) moaning has been addressed, green tests => RTBC.
Comment #15
longwaveAlso realised we can likely remove the NULL, NULL special case once #2838474: Remove dependency of "file_system" service on "logger" lands.
Comment #16
catchMerge conflicts on the MR.
Comment #17
longwaveLet's postpone this on #2838474: Remove dependency of "file_system" service on "logger" as that makes this easier anyway.
Comment #19
andypostRebased after #3416697: Remove install container definitions of FileSystem and StreamWrapperManager
Comment #20
smustgrave commentedLeft 1 question on the MR.
Comment #21
longwaveFollowing #3416697: Remove install container definitions of FileSystem and StreamWrapperManager 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.
Comment #22
andypostComment #23
andypostRemoved the condition, ++ to not add CR as we do all other places
Comment #24
longwaveAs the request stack and current user are now always available, we can always inject them into the created LoggerChannel.
Comment #25
andypostI bet we should provide BC to allow old container to work for upgrades at least
Comment #26
longwaveThe constructor will ensure the new properties are always set, so
$this->containeris no longer needed at all?Comment #27
andypostGood point - fixed
Comment #28
smustgrave commentedInjection 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.
Comment #29
catchQuestion on the MR.
Comment #30
longwaveResponded to the feedback.
Comment #31
needs-review-queue-bot commentedThe 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.
Comment #32
longwaveRebased, responded to feedback.
Comment #33
longwaveGiven there were no changes here, being bold and marking RTBC to get this back with other committers for re-review.
Comment #34
alexpottWhat's the plan for things like:
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.
Comment #35
longwaveI think we can add BC for all of those cases, it might be a bit awkward but looks doable.
Comment #36
longwaveAdded BC and deprecations for calling
get()without the constructor being called,setContainer()being called, and$this->containerbeing accessed, which I think covers all the cases in #34.Out of time for now, still needs a change record.
Comment #37
andypostAdded few comments but the BC is the main concerns
Comment #38
longwave@andypost good questions/suggestions, thank you - responded in MR
Comment #39
andypostThank you! Then it's ready, follow-up to remove bc can be filled later
Comment #42
catchThe 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