LogMessageParser breaks messages containing braces

+++ b/composer.json
@@ -5,7 +5,8 @@
-        "wikimedia/composer-merge-plugin": "^1.4"
+        "wikimedia/composer-merge-plugin": "^1.4",
+        "php-amqplib/php-amqplib": "^2.6"
     },
     "replace": {
         "drupal/core": "^8.4"
@@ -60,5 +61,9 @@

@@ -60,5 +61,9 @@
             "type": "composer",
             "url": "https://packages.drupal.org/8"
         }
-    ]
+    ],
+    "require-dev": {
+        "drush/drush": "^9",
+        "drupal/devel": "1.x-dev"
+    }

It seems this changes are unintended. And probably the cause of the test failure.

Added test case to patch from #6 and a test only patch.

It would be even nicer to add a more generic test using a test logger backend so that this handling would be more generic.

There is an intent of make a generic logger here: #2862282: Add a test logger channel for test base classes to use. In order to have a unified interface with at least dblog, I proposed to finish first #2458191: Provide a storage backend for dblog module

+++ b/core/modules/dblog/tests/src/Functional/DbLogTest.php
@@ -119,6 +119,23 @@ public function testLogEventPage() {
+   * Test individual log event page.

This docs seems duplicated, and does not illustrate the idea behind this test.

So I

- Added test for LogMessageParser service.
- Fixed the copy/paste comment in the new DBLog test

Uploadet, patch, test only patch and interdiff.

You could argue that DbLog test is not strictly needed, since we have a test in the parser class. Since we did write the code and the DbLog was were the bug surfaced, I think that it makes sense to keep the test to verify that the change fixed the issue.

Updating status back to NR - seems like test failed due to some database issues on test machine.

1. +++ b/core/lib/Drupal/Core/Logger/LogMessageParser.php
   @@ -17,7 +17,7 @@ public function parseMessagePlaceholders(&$message, array &$context) {
          // Transform PSR3 style messages containing placeholders to
   

   This comment now also includes twigs placeholders.

2. +++ b/core/modules/dblog/tests/src/Functional/DbLogTest.php
   @@ -119,6 +119,23 @@ public function testLogEventPage() {
   +    $this->drupalLogin($this->adminUser);
   

   If we are going to test this. I would like to see also all the other placeholders like @placeholder, %placeholder and :placeholder

   Those cases are covered here #2760031: Log messages won't appear translated if they have special characters in them. My proposal then is remove the method testMessageParsing from this patch and move the twig case to #2760031 after this patch be committed.

3. +++ b/core/tests/Drupal/Tests/Core/Logger/LogMessageParserTest.php
   @@ -63,6 +63,11 @@ public function providerTestParseMessagePlaceholders() {
   +      // Message with PSR3 and twig style messagea.
   

   There is a typo at the end of this string.

18.1 => {{foo}} and {foo} are both PSR3 placeholders it's basically PLACEHOLDER and {PLACEHOLDER}, so no need to update.
18.2 => What we are testing is convention of {{foo}} => {@foo} - the output of other kind out placeholders are outside of the scope for this issue
18.3 => I corrected typo by removing the new test and simply changing the double PSR3 tests to have single and double {} variants.

New files attached

Ops forgot to change status

So, I've been playing with this patch and seems is working fine. However, when I try this new patch together with #2760031: Log messages won't appear translated if they have special characters in them, I see the following message:

Deprecated: Invalid placeholder (path) in string: Incorrect parameter {bar} in path /baz: horse in /var/www/drupal/core/lib/Drupal/Component/Render/FormattableMarkup.php on line 238

Deprecated: Invalid placeholder (path) in string: Incorrect parameter {bar} in path /baz: horse in /var/www/drupal/core/lib/Drupal/Component/Render/FormattableMarkup.php on line 238

Which makes sense since {{ value }} cannot be translated See: https://www.drupal.org/node/2047135

Therefore, if we commit this feature, as is (which works) then #2760031: Log messages won't appear translated if they have special characters in them will be blocked forever.

I'm attaching the patch with the 2 patches in the same file.

#22 Merging two issues is probably a bad idea. This issue doesn't touch tranlations of variables. Today {foo} works but {{foor}} doesn't - since {{foo}} is converted to @{foo} instead of {@foo}.

If this issue gives new issues to #2760031, we should handle it there and try to merge these two issues as they aren't related to each other.

I'm removing the patch uploaded in #22 so we can continue with the proposed fix #19.

The patch in #19 looks good. Simple regex fix, and the tests seem to cover it correctly.

Note: As per https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-3-logg...,

Placeholder names SHOULD be composed only of the characters A-Z, a-z, 0-9, underscore _, and period .. The use of other characters is reserved for future modifications of the placeholders specification.

Should we make this "SHOULD" mandatory and use [A-Za-z0-9_.] instead of [^\{}]? Other than that I believe this is ready.

Merging two issues is probably a bad idea.

I know, my intention just was to point out that the this patch has side effects.

If this issue gives new issues to #2760031, we should handle it there and try to merge these two issues as they aren't related to each other.

I'm tagging this with Needs framework manager review to decide if we want to decide that logs cannot be translated anymore. I might be wrong, but as I mentioned in #22, showing {{ twig }} will make impossible to translate logs.

Long time passed since I reviewed this issue. My concerns about translations were not valid. I manually applied the patch (does not apply anymore) and managed to translate the log without issues.

This seems good to go in my opinion. But the patch does not apply anymore to 9.3.x

Re-rolled patch against 9.3.x. Please review.

The patch that needs a reroll is this one. #19

Re-rolled the patch in #19 as requested in #35, thanks!

Thanks @ankithashetty

I think this looks good. There is a question about if the current pattern should be modified, but since this fix the original issue I think this qualifies as a follow up with more tests. #25

Committed and pushed f653102333 to 9.3.x and 0865322d5b to 9.2.x. Thanks!

+++ b/core/lib/Drupal/Core/Logger/LogMessageParser.php
@@ -17,7 +17,7 @@ public function parseMessagePlaceholders(&$message, array &$context) {
-      $message = preg_replace('/\{(.*)\}/U', '@$1', $message);
+      $message = preg_replace('/\{([^\{}]*)\}/U', '@$1', $message);

The escaping here is a a little inconsistent but it doesn't matter. I think curly braces only have meaning if there is a number after them so quite possibly all the escaping is unnecessary which would helpful with readability.

Wow, 4 years later and it's in, I forgot I ever worked on this, thanks @dagmar for doing the work to get this over the finish line :)