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

Reference: https://www.drupal.org/core/beta-changes
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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

Maouna’s picture

Assigned: Unassigned » Maouna
Maouna’s picture

Status: Active » Needs review
FileSize
3.77 KB

Here is the patch.

Status: Needs review » Needs work

The last submitted patch, 3: 2571521-Logger-ControllerBase-3.patch, failed testing.

Maouna’s picture

Assigned: Maouna » Unassigned
Status: Needs work » Needs review

In the end the patch passed the test, so setting it to needs review.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine for me. It makes good things easy, and logs are always a good thing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: 2571521-Logger-ControllerBase-3.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: 2571521-Logger-ControllerBase-3.patch, failed testing.

sdstyles’s picture

Status: Needs work » Needs review
FileSize
3.9 KB

Patch #3 could not be applied because new parameter was added in SearchController.php __constructor()

dawehner’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Let's stop here, if possible. Thank you for getting this step done.

Added a beta evaluation.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

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

dawehner’s picture

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

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

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

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

Fabianx’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/search/src/Controller/SearchController.php
@@ -49,14 +49,12 @@ class SearchController extends ControllerBase {
-    $this->logger = $logger;
+    $this->logger = $this->logger('search');

I find it confusing to have a logger() function and a logger property.

$this->getLogger('channel')

maybe?

alexpott’s picture

@Fabianx that is a good point

alexpott’s picture

Status: Needs review » Needs work
Maouna’s picture

Assigned: Unassigned » Maouna
Maouna’s picture

Assigned: Maouna » Unassigned
Status: Needs work » Needs review
FileSize
2.04 KB
3.91 KB

Changed the patch according to #15.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
dawehner’s picture

Status: Reviewed & tested by the community » Needs work

Mh, i'm not convinced.

  • keyValue()
  • cache()
  • config()

All of them have pretty much the same problem. Let's use loggerFactory + logger()

maris.abols’s picture

Assigned: Unassigned » maris.abols
maris.abols’s picture

@dawehner what do you mean with your enumeration of these functions? I cannot see these functions in latest patch.

Maouna’s picture

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

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

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

Agreed

maris.abols’s picture

Assigned: maris.abols » Unassigned

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 2571521-Logger-ControllerBase-19.patch, failed testing.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.91 KB

Rerolled

cilefen’s picture

Would it make sense to set a default for $channel in getLogger?

dawehner’s picture

Does the generic integration into the logging system provides such one?

cilefen’s picture

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

+++ b/core/lib/Drupal/Core/Controller/ControllerBase.php
@@ -259,6 +266,23 @@ protected function languageManager() {
+  protected function getLogger($channel) {
+    if (!$this->loggerFactory) {
+      $this->loggerFactory = $this->container()->get('logger.factory');
+    }
+    return $this->loggerFactory->get($channel);
+  }

Instead I am proposing getLogger($channel = 'controller') so there is a sensible default on this helper function.

dawehner’s picture

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

catch’s picture

Version: 8.0.x-dev » 8.1.x-dev

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

dawehner’s picture

So would the ControllerBase only change committable to 8.0.x? It improves the developer experience ...

catch’s picture

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

dawehner’s picture

@catch
I thought we don't treat constructors as any kind of supported API?

catch’s picture

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

xjm’s picture

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

dawehner’s picture

Well you know, its something people would profit now ... but well, let's just wait then.

dawehner’s picture

Well you know, its something people would profit now ... but well, let's just wait then.

  • catch committed e6a360b on 8.1.x
    Issue #2571521 by Maouna, dawehner, sdstyles: Make the logger available...
catch’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs change record

Committed/pushed to 8.1.x, thanks!

Although having done so, I just realised this needs a change record for the addition.

dawehner’s picture

So here is a general question. Ideally we would adapt the existing one, but this then would not be valid for 8.0.x

catch’s picture

Yeah I think we need new change records for anything in 8.1.x but not 8.0.x.

  • catch committed 6380357 on
    Issue #2571521 by Maouna, dawehner, sdstyles: Make the logger available...

  • catch committed 7f855c9 on 8.0.x
    Revert "Issue #2571521 by Maouna, dawehner, sdstyles: Make the logger...

Status: Fixed » Closed (fixed)

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

fgm’s picture

Sorry 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 ?

cilefen’s picture

$this->loggerChannel = $this->getLogger('my-channel');