Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Logging is quite a common task actually
Proposed resolution
Let's expose the logger, basically, provide \Drupal::logger()
on ContainerBase
Remaining tasks
User interface changes
API changes
Data model changes
Beta phase evaluation
Issue category | Task because its about improving DX |
---|---|
Issue priority | Major because ... Critical/Not critical because ... |
Disruption | Its an addition so it doesn't break anyone's code. |
Comment | File | Size | Author |
---|---|---|---|
#28 | 2571521-28.patch | 3.91 KB | dawehner |
#10 | 2571521-Logger-ControllerBase-10.patch | 3.9 KB | sdstyles |
Comments
Comment #2
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedComment #3
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedHere is the patch.
Comment #5
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedIn the end the patch passed the test, so setting it to needs review.
Comment #6
dawehnerLooks fine for me. It makes good things easy, and logs are always a good thing.
Comment #10
sdstyles CreditAttribution: sdstyles at FFW commentedPatch #3 could not be applied because new parameter was added in SearchController.php __constructor()
Comment #11
dawehnerLet's stop here, if possible. Thank you for getting this step done.
Added a beta evaluation.
Comment #12
alexpottI'm not sure that we should be making this change - if a controller needs a logger it should be injecting it - no? It looks really odd to be removing properly injected code. I'm inclined to "won't fix" this.
Comment #13
dawehnerWell, my personal opinion is that logging is something, much like string translations, are additional stuff, something which is not the actual business logic of a controller.
The central piece of a controller is to produce the output, not the logging.
Comment #14
alexpott@dawehner I see what you are saying and yes this is an application like service similar to string translation. In a different world we would have not had a controllerbase but had an injectable application which would provide these type of services. Changing my mind - let's go ahead and do this.
Comment #15
Fabianx CreditAttribution: Fabianx as a volunteer commentedI find it confusing to have a logger() function and a logger property.
$this->getLogger('channel')
maybe?
Comment #16
alexpott@Fabianx that is a good point
Comment #17
alexpottComment #18
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedComment #19
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedChanged the patch according to #15.
Comment #20
Fabianx CreditAttribution: Fabianx as a volunteer commentedComment #21
dawehnerMh, i'm not convinced.
All of them have pretty much the same problem. Let's use loggerFactory + logger()
Comment #22
maris.abols CreditAttribution: maris.abols commentedComment #23
maris.abols CreditAttribution: maris.abols commented@dawehner what do you mean with your enumeration of these functions? I cannot see these functions in latest patch.
Comment #24
Maouna CreditAttribution: Maouna at bio.logis Genetic Information Management GmbH commentedThen let's go with the patch from #10. While writing the first patch for this issue, I was following the style of config() in order to keep it consistent.
Comment #25
dawehnerAgreed
Comment #26
maris.abols CreditAttribution: maris.abols commentedComment #28
dawehnerRerolled
Comment #29
cilefen CreditAttribution: cilefen commentedWould it make sense to set a default for $channel in getLogger?
Comment #30
dawehnerDoes the generic integration into the logging system provides such one?
Comment #31
cilefen CreditAttribution: cilefen commentedI think you are asking me rhetorically. If you use logger.factory, you must specify a channel name.
There are container services for some generic channel names, like 'system', 'php', and 'form'. Maybe 'controller' would be appropriate here. We could add a logger.channel.controller service for 'controller' and use that instead. The trouble is, it looks like there is no public channel setter on the pre-built channel services so you would be locked into say, 'controller', which would not be very flexible. So that is not ideal.
Instead I am proposing
getLogger($channel = 'controller')
so there is a sensible default on this helper function.Comment #32
dawehnerWell, unless there is a huge advantage, which I don't see, I would say we should just educate people about the proper way of using it.
Comment #33
catchMoving to 8.1.x since this is an API addition to the base class, and it changes constructor params for a controller.
Patch looks great to me.
Comment #34
dawehnerSo would the ControllerBase only change committable to 8.0.x? It improves the developer experience ...
Comment #35
catchIt's an API addition, so according to a strict reading of semver I think it would normally be 8.1.x
However for me, I think we can also commit it to 8.0.x if we mark it @internal - 8.1.x would un-internal it.
Then in the very unlikely scenario we had to revert the patch in 8.1.x and 8.0.x, 8.1.x isn't supported yet, and it's not-an-API in 8.0.x either.
Contrib modules can still use it if it's @internal - that's just warning not a proscription.
Comment #36
dawehner@catch
I thought we don't treat constructors as any kind of supported API?
Comment #37
catch@dawehner we don't, but I think generally for things we've documented as @internal, we'd tend towards doing them only in minor releases anyway. So no bc-layer, but for @internal bc breaks they'd tend to happen in minor versions.
At least I'd default to that, and then if there's something urgent that requires that kind of change, consider it for patch releases individually.
Comment #38
xjmFor me, we should commit this to 8.1.x now but not backport it to 8.0.x unless it is a hard blocker for some contributed project (which I don't think it should be; one can work around it).
Constructors are allowed to change in minors and I agree we can change it in a minor version for that reason.
Comment #39
dawehnerWell you know, its something people would profit now ... but well, let's just wait then.
Comment #40
dawehnerWell you know, its something people would profit now ... but well, let's just wait then.
Comment #42
catchCommitted/pushed to 8.1.x, thanks!
Although having done so, I just realised this needs a change record for the addition.
Comment #43
dawehnerSo here is a general question. Ideally we would adapt the existing one, but this then would not be valid for 8.0.x
Comment #44
catchYeah I think we need new change records for anything in 8.1.x but not 8.0.x.
Comment #48
fgmSorry to notice this just now, but couldn't we just cache the actual logger (channel) on the instance instead of having to get() it each time from the channel factory ?
Comment #49
cilefen CreditAttribution: cilefen commented$this->loggerChannel = $this->getLogger('my-channel');