Refactor how dblog module is rendering links in event details

Shall we postpone the other issues on this one?

Posting my patch from the other issue.

Combined patch from 2755497 and 2858182.

+++ b/core/lib/Drupal/Core/Logger/LoggerChannel.php
@@ -110,7 +110,9 @@ public function log($level, $message, array $context = []) {
-      $context['request_uri'] = $request->getUri();
+      if ($request->getHost()) {
+        $context['request_uri'] = $request->getUri();
+      }

IMHO this should document why we need this if here.

Added new method to format the referer URL.

Needs work for #7

+++ b/core/modules/dblog/src/Controller/DbLogController.php
@@ -420,4 +447,23 @@ public function topLogMessages($type) {
+  protected function cleanReferer(Url $url) {

This method is not being used anymore.

Here is updated patch as per suggestions in #10. Removed unused protected function cleanReferer(Url $url) from patch and updated test cases so that it does not require changes mentioned in #7.

By mistake uploaded the same patch of #7. Here is updated patch.

1. +++ b/core/modules/dblog/src/Controller/DbLogController.php
   @@ -368,6 +372,22 @@ public function formatMessage($row) {
   +   * Formats a uri to link if valid.
   

   Should give context here to explain when the link might not be valid -- because it's an empty string? because the logger passed in just a plain text message? Something else?

2. +++ b/core/modules/dblog/src/Controller/DbLogController.php
   @@ -368,6 +372,22 @@ public function formatMessage($row) {
   +   * @return string|\Drupal\Core\Link
   +   *   The formatted link.
   

   Needs to say what you get back if it's not valid.

Updated method docblock and return type description as per #19

Thanks, @dagmar for feedback.

Some log attributes like Referer or Location can be rendered as links if they are correctly formatted. This function returns a link version of an uri if it can be successfully rendered as a link. It fallback to the string version of the uri otherwise.

and

Return a Link object if the uri can be converted as a link. In case of empty uri, or invalid, fallback to the plain string.

I think these are explanatory enough. I'll try to add test for location in DbLogTest::testLogEventPage.

Here is updated patch as per suggestion in #22

Damn!! missed to hit upload button to upload patch before uploading interdiff. Here we go with both patch and interdiff.

Updated patch as per #29

+++ b/core/modules/dblog/src/Controller/DbLogController.php
@@ -243,7 +245,6 @@ public function eventDetails($event_id) {
-      $message = $this->formatMessage($dblog);

This change is not necessary.

I think we can keep this change to maintain consistency. $message variable is also being used only once which can be replaced by $this->formatMessage($dblog).

+++ b/core/modules/dblog/src/Controller/DbLogController.php
@@ -368,6 +369,28 @@ public function formatMessage($row) {
+   * uri if it can be successfully render as a link. It fallback to the string

s/render/rendered - This is valid.
s/fallback/fallbacks - IDE gives typo error on this change.

+++ b/core/modules/dblog/src/Controller/DbLogController.php
@@ -368,6 +369,28 @@ public function formatMessage($row) {
+   *   empty uri or invalid, fallback to plain string.

s/fallback/fallbacks - IDE gives typo error on this change.

I'll update test case according to point 4 and 5.

Here is updated patch.

1. +++ b/core/modules/dblog/src/Controller/DbLogController.php
   @@ -243,7 +245,6 @@ public function eventDetails($event_id) {
   -      $message = $this->formatMessage($dblog);
   
   @@ -263,15 +264,15 @@ public function eventDetails($event_id) {
   -          $message,
   +          $this->formatMessage($dblog),
   

   This change is not required for the change.

2. +++ b/core/modules/dblog/src/Controller/DbLogController.php
   @@ -368,6 +369,28 @@ public function formatMessage($row) {
   +   * @param string $uri
   

   This can be a null so the typehint should be a string|null

3. +++ b/core/modules/dblog/tests/src/Functional/DbLogTest.php
   @@ -114,10 +114,57 @@ public function testLogEventPage() {
   +   * Test individual log event page with missing log attributes.
   ...
   +      'referer' => 'http://example.org?dblog=1',
   

   We should also test a log entry where these are not set at all. The test hints that it is doing a test where there is no request_uri or referer. But it doesn't appear to be.

4. +++ b/core/tests/Drupal/Tests/Core/Logger/LoggerChannelTest.php
   @@ -106,11 +106,20 @@ public function providerTestLog() {
   -    $request_mock = $this->getMock('Symfony\Component\HttpFoundation\Request', ['getClientIp']);
   +    $request_mock = $this->getMock('Symfony\Component\HttpFoundation\Request', ['getClientIp', 'getHost', 'getUri']);
        $request_mock->expects($this->any())
          ->method('getClientIp')
          ->will($this->returnValue('127.0.0.1'));
   +    $request_mock->expects($this->any())
   +      ->method('getHost')
   +      ->will($this->returnValue(NULL));
   +    $request_mock->expects($this->any())
   +      ->method('getUri')
   +      ->will($this->returnValue('http://:/'));
        $request_mock->headers = $this->getMock('Symfony\Component\HttpFoundation\ParameterBag');
   +    $request_mock->headers->expects($this->any())
   +      ->method('get')
   +      ->will($this->returnValue('Drupal'));
    
        // No request or account.
        $cases[] = [
   @@ -127,6 +136,15 @@ function ($context) {
   
   @@ -127,6 +136,15 @@ function ($context) {
          NULL,
          $account_mock,
        ];
   +    // Request with referer but no hostname. Request with no hostname will have
   +    // request_uri = 'http://:/'.
   +    $cases [] = [
   +      function ($context) {
   +        return $context['referer'] == 'Drupal' && $context['request_uri'] == 'http://:/';
   +      },
   +      $request_mock,
   +      NULL,
   +    ];
   

   This should be in a separate issue it is testing code that is not changed by the fix as far as I can see so it seems confusing.

New patch according to #37.

I used generateLogEntries method to inject fake log data and Database::getConnection()->query($query)->fetchField() to get the last log.

Re #41

I think this block of code can be replaced with the new generateLogEntries method.

I think we can change this also in the other tests. I've created issue.
#3039207: Use the generateLogEntries method in DBLogTest

1. +++ b/core/modules/dblog/src/Controller/DbLogController.php
   @@ -369,6 +371,28 @@ public function formatMessage($row) {
   +   * Formats a uri string to link if valid.
   ...
   +  protected function formatLink($uri) {
   

   Let's call this createLink and change the text to Creates a Link object if the provided URI is valid.

2. +++ b/core/modules/dblog/src/Controller/DbLogController.php
   @@ -369,6 +371,28 @@ public function formatMessage($row) {
   +   * Some log attributes like 'referer' or 'location' can be rendered as links
   +   * if they are correctly formatted. This function returns a link version of an
   +   * uri if it can be successfully rendered as a link. It fallback to the string
   +   * version of the uri otherwise.
   

   This text is a bit verbose and repeats information from the one liner. And doesn't deal with the null situation. I'm not sure it is necessary at all.

3. +++ b/core/modules/dblog/src/Controller/DbLogController.php
   @@ -369,6 +371,28 @@ public function formatMessage($row) {
   +   * @param string|null $uri
   +   *   The uri string to convert into link if valid.
   ...
   +   *   Return a Link object if the uri can be converted as a link. In case of
   +   *   empty uri or invalid, fallback to plain string.
   

   Can return NULL no? Could say fallback to the provided $uri. also @return \Drupal\Core\Link|string|null

4. As per the issues marked duplicate of this - this looks like a bug fix.

Re #45.1 and #45.2. A new description is enough, so I have removed a long text.

   /**
-   * Formats a uri string to link if valid.
-   *
-   * Some log attributes like 'referer' or 'location' can be rendered as links
-   * if they are correctly formatted. This function returns a link version of an
-   * uri if it can be successfully rendered as a link. It fallback to the string
-   * version of the uri otherwise.
+   * Creates a Link object if the provided URI is valid.
    *

Committed and pushed 511bdacdd2 to 8.8.x and 2b5e56a9cc to 8.7.x. Thanks!