LogMessageParser::parseMessagePlaceholders() needs to switch bang placeholder to colon placeholder

+++ b/core/lib/Drupal/Core/Logger/LogMessageParser.php
@@ -34,7 +34,7 @@ public function parseMessagePlaceholders(&$message, array &$context) {
       }
-      if (!empty($key) && ($key[0] === '@' || $key[0] === '%' || $key[0] === '!')) {
+      if (!empty($key) && ($key[0] === '@' || $key[0] === '%' || $key[0] === ':')) {
         // The key is now in \Drupal\Component\Utility\SafeMarkup::format() style.

Right, other ones would pass then to t() in Dblog, which is IMHO still crazy, but we do that.

Some simple test would be nice!

#2481349: Prevent the use of placeholders that cannot be converted into strings when creating logs here we have tests for this, test are not passing at this moment. Can we set this to duplicate and work on the other issue?

@dagmar
I'm a bit confused because this fix doesn't appear in the issue you linked to.

@dawehner Please take a look to comment #23 https://www.drupal.org/node/2481349#comment-10905482 and #27

This still needs tests.

I'm closing this issue in favor of #2760031: Log messages won't appear translated if they have special characters in them which contains all the tests that this issue requires.

This hasn't been fixed in 5 years. Let's provide a simple test that expose the issue and get someone write the patch. The patch that fixes this issue is in #2. We need to provide a combined patch with the test provided here and the fix provided in #2.

Fixed some typos in the comment.

More typos...

I finally just stumbled into this bug, after noticing that two of my logger modules are not parsing message placeholders correctly (of course it also affects dblog and syslog modules).

Combining the test patch from #15 with the bug fix patch from #2

I tested the patch with dblog, Drush logger and two contrib loggers; log messages with <a href=":url"> now render correctly :) Marking this RTBC since I didn't write either parts of the patch; also fixing a typo in the issue summary.

Just added template to the IS.

+++ b/core/tests/Drupal/Tests/Core/Logger/LogMessageParserTest.php
@@ -63,6 +63,11 @@ public function providerTestParseMessagePlaceholders() {
+      // Message with :placeholder and !placeholder now removed.

This is not very clear. :placeholder is not removed.

I think it is okay to just remove the ! - I'm not sure that we need to support : - maybe it is best for consistency but we've survived this long without it...

I think this should read something like
// Test removal unexpected placeholders like ! while allowed placeholders beginning with @, % and : are preserved.

@alexpott,
Made changes as per comment #22.
Needs review.

@Ratan Priya can you wrap the long comment onto multiple lines at 80 characters? Also, the grammar is off - should be

Test removal of unexpected placeholders

@fgm - likely a bug introduced by: #3226881: Do not automatically credit people who upload files to an issue - we'll investigate

Crediting @fgm

+++ b/core/tests/Drupal/Tests/Core/Logger/LogMessageParserTest.php
@@ -63,6 +63,11 @@ public function providerTestParseMessagePlaceholders() {
+      // Test removal unexpected placeholders like ! while allowed placeholders beginning with @, % and : are preserved.
+      [

The comment should wrap at 80 characters. I will fix this if this patch is rtbc.

@fgm - credit is not saved until a maintainer comments. They may update as they go, and should decide on final credit as the issue is fixed, credit is not complete until the issue is fixed. In this case, you were indeed first credited by #27. Maintainers had commented previously, so they did not choose to credit at the time. This could have been an explicit decision, or could have been the defaults at the time did not suggest crediting people who uploaded patches. Since this was seven years ago, and the system worked as alexpott has credited you, this is not worth spending any more time investigating.

Needs work per #24 . This requires a minor grammar fix, and wrap the comment to 80 characters.

@dagmar i made changes as per comment #24 needs review.

Thanks @pradhumanjainOSL According to https://www.drupal.org/docs/develop/standards/api-documentation-and-comm... use of /* */ comments is ok but discouraged within functions.

Maybe @alexpott wants to fix that as he mentioned in #28

Anyways this looks good.

Backported to 9.5.x as a low risk bug fix. Thanks!

diff --git a/core/tests/Drupal/Tests/Core/Logger/LogMessageParserTest.php b/core/tests/Drupal/Tests/Core/Logger/LogMessageParserTest.php
index 13cb673155..8d1347bc57 100644
--- a/core/tests/Drupal/Tests/Core/Logger/LogMessageParserTest.php
+++ b/core/tests/Drupal/Tests/Core/Logger/LogMessageParserTest.php
@@ -63,10 +63,9 @@ public function providerTestParseMessagePlaceholders() {
         ['message' => 'Test {with} two {{encapsuled}} strings', 'context' => ['with' => 'together', 'encapsuled' => 'awesome']],
         ['message' => 'Test @with two {@encapsuled} strings', 'context' => ['@with' => 'together', '@encapsuled' => 'awesome']],
       ],
-      /**
-      * Test removal of unexpected placeholders like ! while allowed placeholders
-      * beginning with @, % and : are preserved.
-      **/
+
+      // Test removal of unexpected placeholders like ! while allowed
+      // placeholders beginning with @, % and : are preserved.
       [
         ['message' => 'Test placeholder with :url and old !bang parameter', 'context' => [':url' => 'https://drupal.org', '!bang' => 'foo bar']],
         ['message' => 'Test placeholder with :url and old !bang parameter', 'context' => [':url' => 'https://drupal.org']],

Fixed code comments on commit.

Discussed with @longwave we agreed to backport this to 9.4.x

Committed 59143d8 and pushed to 9.4.x. Thanks!