Make the logger available on the controllerBase

Here is the patch.

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

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

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

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

@Fabianx that is a good point

Changed the patch according to #15.

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

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.

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

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.

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.

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

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.

Committed/pushed to 8.1.x, thanks!

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

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

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 ?

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