Problem/Motivation

todo

Steps to reproduce

Todo

Proposed resolution

todo

Remaining tasks

Update issue summary.
Review code
Manual test
Commit

User interface changes

Translation on log messages page

API changes

NA

Data model changes

NA

Release notes snippet

NA

Original Post

I haven't tested this, but @hgoto has done some equivalent testing on a Drupal 7 patch that would have done what Drupal 8 does now (see #2459339: Log messages should be XSS filtered on display) and the problem occurred then.

Because https://api.drupal.org/api/drupal/core%21modules%21dblog%21src%21Control... does this:

$message = $this->t(Xss::filterAdmin($row->message), unserialize($row->variables));

It suggests that if the message has any characters which get altered by Xss:filterAdmin() (such as <, > or &) then the message won't be properly translated, and the entire message will appear in English when viewing the logs even when the site is being viewed in a different language.

The solution of running Xss::filterAdmin() on the result of $this->t() instead was discussed in the above issue. It was rejected at the time, but I'm not sure the reasons for rejecting it are relevant anymore. Doing that should fix this.

@hgoto has an initial Drupal 8 patch for this at #2459339-29: Log messages should be XSS filtered on display

CommentFileSizeAuthor
#95 2760031-95.patch7.8 KBAbhisheksingh27
#94 2760031-94.patch7.52 KBAbhisheksingh27
#92 2760031-92.patch8.01 KBsmustgrave
#92 interdiff-83-92.txt1.16 KBsmustgrave
#91 Screenshot from 2022-09-05 18-11-38.png115.46 KBAnjali Rathod
#91 Screenshot from 2022-09-05 18-07-50.png146.41 KBAnjali Rathod
#90 2760031-90.patch14.14 KBsmustgrave
#89 2760031-89.patch14.13 KBsmustgrave
#89 interdiff-87-89.txt9.38 KBsmustgrave
#87 2760031-87.patch7.46 KBsmustgrave
#83 interdiff_76-83.txt1.72 KBvsujeetkumar
#83 2760031-83.patch7.71 KBvsujeetkumar
#82 interdiff_76-82.txt1.48 KBvsujeetkumar
#82 2760031-82.patch7.95 KBvsujeetkumar
#76 interdiff-76.txt0 bytesKapilV
#76 2760031-76.patch7.57 KBKapilV
#74 interdiff.txt579 bytesrenatog
#74 2760031.patch7.74 KBrenatog
#69 2760031-69.patch7.56 KBsanjayk
#68 interdiff_65_68.txt1.2 KBsanjayk
#68 2760031-68.patch7.76 KBsanjayk
#67 interdiff_65_66.txt1.2 KBsanjayk
#65 2760031-65.patch7.6 KBsanjayk
#60 2760031-56.patch7.6 KBanushrikumari
#59 2760031-55.patch7.64 KBanushrikumari
#54 2760031-54.patch7.8 KBizus
#54 interdiff_54-53.txt1.88 KBizus
#53 2760031-53.patch7.27 KBizus
#49 Log_Messages_2760031-49.patch6.37 KBgawaksh
#47 interdiff-2760031-42-47.txt1.68 KByogeshmpawar
#47 2760031-47.patch7.25 KByogeshmpawar
#42 2760031-42.patch7.59 KBMerryHamster
#39 interdiff-2760031-35-39.txt1.86 KByogeshmpawar
#39 2760031-39.patch7.67 KByogeshmpawar
#35 log_messages_won_t-2760031-35.patch7.31 KByogeshmpawar
#29 interdiff-2760031-25-29.txt1.69 KByogeshmpawar
#29 log_messages_won_t-2760031-29.patch7.17 KByogeshmpawar
#25 interdiff-2760031-20-24.txt1.24 KBhgoto
#25 xss-dblog-2760031-24.patch7.97 KBhgoto
#25 xss-dblog-2760031-24-test_only.patch5.47 KBhgoto
#20 xss-dblog-2760031-20-test_only.patch5.49 KBhgoto
#20 xss-dblog-2760031-20.patch7.3 KBhgoto
#14 interdiff-2760031-8-14.txt4.23 KBhgoto
#14 xss-dblog-2760031-14.patch6.7 KBhgoto
#14 xss-dblog-2760031-14-test_only_should_fail.patch5.03 KBhgoto
#8 interdiff-2760031-6-8.txt1.44 KBhgoto
#8 xss-dblog-2760031-8.patch3.81 KBhgoto
#6 interdiff-2760031-5-6.txt1.94 KBhgoto
#6 xss-dblog-2760031-6.patch3.79 KBhgoto
#5 interdiff-2760031.txt2.05 KBhgoto
#5 xss-dblog-2760031-5.patch2.9 KBhgoto
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein created an issue. See original summary.

David_Rothstein’s picture

Status: Active » Needs work

"Needs work" since there is a patch, which can be copied here.

Adding credit for @hgoto.

David_Rothstein’s picture

Issue summary: View changes
hgoto’s picture

Status: Needs work » Needs review
FileSize
2.9 KB
2.05 KB

@David_Rothstein, thank you for organizing the information and creating the issue.

I created a new patch for this issue. I revised some points from the previous patch #29 of #2459339: Log messages should be XSS filtered on display.

1. Changed the following line to avoid double escaping. For example, <script>alert('foo');</script> <strong>Lorem ipsum</strong> is shown as alert('foo'); &lt;strong&gt;Lorem ipsum&lt;/strong&gt; in the detailed page table if we don't change this line.

@@ -267,7 +267,7 @@ public function eventDetails($event_id) {
         ),
         array(
           array('data' => $this->t('Message'), 'header' => TRUE),
-          $message,
+          array('data' => array('#markup' => $message)),
         ),
         array(
           array('data' => $this->t('Severity'), 'header' => TRUE),

2. Changed back the @return comment of the method formatMessage().

3. Splitted the test from the testOverviewLinks() and created a dedicated test case testLogEventPageMessageEscaped().

As for the point 1 above, I'm not confident that this approach is the best. I'd like someone to review this considering twig and table template.

hgoto’s picture

I'msorry, the patch #5 doesn't consider David_Rothstein's guide in comment #36 on #2459339: Log messages should be XSS filtered on display.

What is the reason for making all those changes to the generateLogEntries() method in the tests? Since we're just trying to test a specific log message here, I would think having the new test call watchdog() directly would be better.

Surely, generateLogEntries() is not necessary to use here and I revised the test case to make it simpler.

Fabianx’s picture

Status: Needs review » Needs work

Hmm, would the data => #markup change not always escape the thing with XSS::filterAdmin() again?

I think it would be better to always return a Markup object instead similar to how https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21... would do it.

hgoto’s picture

Status: Needs work » Needs review
FileSize
3.81 KB
1.44 KB

@Fabianx, thank you for the review and assist.

I see. I changed the return value of formatMessage() to a Markup object. Surely it looks better.

And data => #markup was necessary to avoid double escape in the patch #6 but it's no longer needed after replacing the return value of formatMessage().

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC - looks good to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: xss-dblog-2760031-8.patch, failed testing.

hgoto’s picture

Status: Needs work » Reviewed & tested by the community

The auto test succeeded once and failed later with CI error. So I believe it's OK to make this status RTBC again.

Maybe this is related to #2749955: Random fails in UpdatePathTestBase tests

xjm’s picture

You can retest patches by clicking the "Add new test" link in tiny characters underneath the last test (not obvious, I know). :)

The CI error is actually a problem with the infrastructure itself, not random fails. So usually when that happens the patch should just be retested. Doing so now.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Thanks for working on this issue.

  1. +++ b/core/modules/dblog/src/Tests/DbLogTest.php
    @@ -117,6 +117,32 @@ public function testLogEventPage() {
    +    // Make sure HTML tags are filtered out in admin/reports/dblog/event/ too.
    +    \Drupal::service('logger.dblog')->log(RfcLogLevel::NOTICE, "<script>alert('foo');</script> <strong>Lorem ipsum</strong>", $context);
    +    $wid = db_query('SELECT MAX(wid) FROM {watchdog}')->fetchField();
    +
    +    $this->drupalGet('admin/reports/dblog/event/' . $wid);
    +    $this->assertResponse(200);
    +    $this->assertNoRaw("<script>alert('foo');</script>");
    +    $this->assertRaw("alert('foo'); <strong>Lorem ipsum</strong>");
    
    @@ -769,13 +795,6 @@ public function testOverviewLinks() {
    -    // Make sure HTML tags are filtered out in admin/reports/dblog/event/ too.
    -    $this->generateLogEntries(1, ['message' => "<script>alert('foo');</script> <strong>Lorem ipsum</strong>"]);
    -    $wid = db_query('SELECT MAX(wid) FROM {watchdog}')->fetchField();
    -    $this->drupalGet('admin/reports/dblog/event/' . $wid);
    -    $this->assertNoRaw("<script>alert('foo');</script>");
    -    $this->assertRaw("alert('foo'); <strong>Lorem ipsum</strong>");
    

    Why are we moving almost exactly the same test from one place to the other, without adding anything? Unless I missed something, what this accomplishes is making testbot install the site an extra time with the same coverage. That said...

  2. +++ b/core/modules/dblog/src/Controller/DbLogController.php
    @@ -347,12 +348,13 @@ public function formatMessage($row) {
         if (isset($row->message) && isset($row->variables)) {
           // Messages without variables or user specified text.
           if ($row->variables === 'N;') {
    -        $message = Xss::filterAdmin($row->message);
    +        $message = $row->message;
           }
           // Message to translate with injected variables.
           else {
    -        $message = $this->t(Xss::filterAdmin($row->message), unserialize($row->variables));
    +        $message = $this->t($row->message, unserialize($row->variables));
           }
    +      $message = Markup::create(Xss::filterAdmin($message));
    

    So I guess that the idea here is that we want to XSS filter after the message is translated, rather than before.

    It still seems a bit overcomplicated and also (in HEAD as well) a misuse of t()... however, before I recommend changes, let's get some test coverage. If the bug is that the messages do not get translated when they are supposed to be, then let's add tests for that -- for messages that should be translated and would not be in HEAD. We would upload a failing test-only patch for the translation, and a passing combined one.

    Once the test coverage is fixed, then we can ensure that any changes we make to the code itself also do what they are supposed to.

Thanks!

hgoto’s picture

@xjm, thank you for your detailed review and comments.

Why are we moving almost exactly the same test from one place to the other, without adding anything? Unless I missed something, what this accomplishes is making testbot install the site an extra time with the same coverage. That said...

As you saw, this doesn't change the coverage. We overused the method generateLogEntries() in the commit http://cgit.drupalcode.org/drupal/commit/?id=a2a4051 and we're going to fix that on D7 in #2459339: Log messages should be XSS filtered on display. I thought it's better to fix it also in D8 and that's why I changed the test without adding coverage. The comment https://www.drupal.org/node/2459339#comment-11363781 may be helpful.

So I guess that the idea here is that we want to XSS filter after the message is translated, rather than before.

It still seems a bit overcomplicated and also (in HEAD as well) a misuse of t()... however, before I recommend changes, let's get some test coverage. If the bug is that the messages do not get translated when they are supposed to be, then let's add tests for that -- for messages that should be translated and would not be in HEAD. We would upload a failing test-only patch for the translation, and a passing combined one.

Once the test coverage is fixed, then we can ensure that any changes we make to the code itself also do what they are supposed to.

I see. If I understand that, we should create a test to confirm there's a bug in HEAD, right.

As the first step, I tried creating a new test-only patch. It's a little complicated and I'd like to upload another patch with working code to compare the results.

I didn't understand the misuse of t() and I'll tackle that point later.

If my understanding is wrong or not enough, please tell me. Thank you.

hgoto’s picture

Status: Needs work » Needs review

The last submitted patch, 14: xss-dblog-2760031-14-test_only_should_fail.patch, failed testing.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dagmar’s picture

Status: Needs review » Needs work

Thanks! @hgoto

+++ b/core/modules/dblog/src/Tests/DbLogTest.php
@@ -117,6 +120,91 @@ public function testLogEventPage() {
+    \Drupal::service('logger.dblog')->log(RfcLogLevel::NOTICE, "<script>alert('foo');</script> <strong>Lorem ipsum</strong>", $context);

You can use ->notice().


+++ b/core/modules/dblog/src/Tests/DbLogTest.php
@@ -117,6 +120,91 @@ public function testLogEventPage() {
+    $translation_variables = ['@something' => 'foo'];

Could you use the 3 types of placeholders here? We have another issue that needs tests regarding this topic. #2617330: LogMessageParser::parseMessagePlaceholders() needs to switch bang placeholder to colon placeholder

+++ b/core/modules/dblog/src/Tests/DbLogTest.php
@@ -22,7 +25,7 @@ class DbLogTest extends WebTestBase {
+  public static $modules = array('dblog', 'node', 'forum', 'help', 'block', 'locale');

We need to use [] now.

hgoto’s picture

@dagmar thank you. Following the review #19, I revised the patch with 3 points improved. I'd like this to be reviewed.

Could you use the 3 types of placeholders here? We have another issue that needs tests regarding this topic. #2617330: LogMessageParser::parseMessagePlaceholders() needs to switch bang placeholder to colon placeholder

I think the changes including the following is enough for that but if I don't understand it correctly, please let me know.

+    $translation_variables = [
+      '@one' => 'foo',
+      '%two' => 'bar',
+      ':three' => 'http://example.com',
+    ];
+

Tests in this patch needs the patch #2617330-2: LogMessageParser::parseMessagePlaceholders() needs to switch bang placeholder to colon placeholder committed in order to pass. (I tested it in my environment.)

Also, I'm sorry, I couldn't create an interdiff. Probably it's because the code around the patch changed since then.

hgoto’s picture

The last submitted patch, 20: xss-dblog-2760031-20.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 20: xss-dblog-2760031-20-test_only.patch, failed testing.

dagmar’s picture

Issue tags: -Needs tests

@hgoto thanks. I marked #2617330: LogMessageParser::parseMessagePlaceholders() needs to switch bang placeholder to colon placeholder as a duplicate of this one.

Could you include the fix from #2617330 in your patch?

hgoto’s picture

@dagmar thanks!

I merged the fix in #2617330 into the last patch. There was another log(RfcLogLevel::NOTICE, ...) left and I also replaced it to notice(...). Auto Tests should pass...

The last submitted patch, 25: xss-dblog-2760031-24-test_only.patch, failed testing.

dagmar’s picture

Status: Needs review » Needs work

Great, green. Almost there.

  1. +++ b/core/modules/dblog/src/Tests/DbLogTest.php
    @@ -117,6 +120,95 @@ public function testLogEventPage() {
    +      'timestamp' => REQUEST_TIME,
    

    I forgot this, the use of REQUEST_TIME is deprecated. https://www.drupal.org/node/2785211

  2. +++ b/core/modules/dblog/src/Tests/DbLogTest.php
    @@ -823,13 +915,6 @@ public function testOverviewLinks() {
    -    // Make sure HTML tags are filtered out in admin/reports/dblog/event/ too.
    -    $this->generateLogEntries(1, ['message' => "<script>alert('foo');</script> <strong>Lorem ipsum</strong>"]);
    -    $wid = db_query('SELECT MAX(wid) FROM {watchdog}')->fetchField();
    -    $this->drupalGet('admin/reports/dblog/event/' . $wid);
    -    $this->assertNoRaw("<script>alert('foo');</script>");
    -    $this->assertRaw("alert('foo'); <strong>Lorem ipsum</strong>");
    

    I would prefer to keep this lines here. The patch is adding 'locale' module as a dependency, that we could abstract into a new test case in the future because it seems only necessary for the new test. Lets keep this 6 lines of code unmodified.

dagmar’s picture

Great, green. Almost there.

  1. +++ b/core/modules/dblog/src/Tests/DbLogTest.php
    @@ -117,6 +120,95 @@ public function testLogEventPage() {
    +      'timestamp' => REQUEST_TIME,
    

    I forgot this, the use of REQUEST_TIME is deprecated. https://www.drupal.org/node/2785211

  2. +++ b/core/modules/dblog/src/Tests/DbLogTest.php
    @@ -823,13 +915,6 @@ public function testOverviewLinks() {
    -    // Make sure HTML tags are filtered out in admin/reports/dblog/event/ too.
    -    $this->generateLogEntries(1, ['message' => "<script>alert('foo');</script> <strong>Lorem ipsum</strong>"]);
    -    $wid = db_query('SELECT MAX(wid) FROM {watchdog}')->fetchField();
    -    $this->drupalGet('admin/reports/dblog/event/' . $wid);
    -    $this->assertNoRaw("<script>alert('foo');</script>");
    -    $this->assertRaw("alert('foo'); <strong>Lorem ipsum</strong>");
    

    I would prefer to keep this lines here. The patch is adding 'locale' module as a dependency, that we could abstract into a new test case in the future because it seems only necessary for the new test. Lets keep this 6 lines of code unmodified.

yogeshmpawar’s picture

Status: Needs work » Needs review
FileSize
7.17 KB
1.69 KB

Updated patch as per comment #27 & also added interdiff.

hgoto’s picture

Status: Needs review » Needs work

@dagmar thank you for your review.

1. Thank you. I didn't notice that. So I believe it's better to replace all REQUEST_TIME in this file, right?

2. That test is covered in testLogEventPageMessageEscaped() and we can remove it. The test method name is testOverviewLinks() and these lines shouldn't be there originally, I think. Please see comment #13 and #14. (If we keep it, it's better to change the method name, isn't it?)

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dagmar’s picture

Issue tags: +Needs reroll, +Novice

1. Thank you. I didn't notice that. So I believe it's better to replace all REQUEST_TIME in this file, right?

Lets skip the replacement of REQUEST_TIME for now, several other issues has shown this change is not required as part of this patch.

2. That test is covered in testLogEventPageMessageEscaped() and we can remove it. The test method name is testOverviewLinks() and these lines shouldn't be there originally, I think. Please see comment #13 and #14. (If we keep it, it's better to change the method name, isn't it?)

I guess you are right. #29 claims it included in the interdiff but actually that's not true.

So the remaining thing for this patch is, rollaback the REQUEST_TIME related changes. and see if the testbot still pass. This will probably need a reroll. Tagging as novice since the remaining changes seems quite simple to implement.

yogeshmpawar’s picture

Assigned: Unassigned » yogeshmpawar
yogeshmpawar’s picture

Assigned: yogeshmpawar » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll, -Novice
FileSize
7.31 KB

Modified the patch as per comment #33 & also this patch failed to apply on 8.5.x branch because of "core/modules/dblog/src/Tests/DbLogTest.php" changed to "core/modules/dblog/tests/src/Functional/DbLogTest.php".

dagmar’s picture

Status: Needs review » Needs work

Cool, Thanks @Yogesh Pawar!

There is one more thing I would like to see in the test coverage. According to this #2952091: Show translatable db log message on the main view, the logs messages are not translated in views. So adding the same checkes that we have now for detail page logs in the logs view should expose the failure.

The approach then could be, take this portion of code and abstract it into a protected method (perhaps assertTranslations?)

+++ b/core/modules/dblog/tests/src/Functional/DbLogTest.php
@@ -118,6 +121,95 @@ public function testLogEventPage() {
+
+    $source_unfiltered = new FormattableMarkup($text_source, $translation_variables);
+    $source_filtered = Xss::filterAdmin(new FormattableMarkup($text_source, $translation_variables));
+    $translation_unfiltered = new FormattableMarkup($text_translation, $translation_variables);
+    $translation_filtered = Xss::filterAdmin(new FormattableMarkup($text_translation, $translation_variables));
+
+    $this->assertNoRaw($source_unfiltered);
+    $this->assertNoRaw($source_filtered);
+    $this->assertNoRaw($translation_unfiltered);
+    $this->assertRaw($translation_filtered);

And then use it twice, one for

+    // Make sure HTML tags are filtered out after translation.
+    $this->drupalGet('admin/reports/dblog/event/' . $wid);
+    $this->assertResponse(200);

and other for

+    // Make sure HTML tags are filtered out after translation.
+    $this->drupalGet('admin/reports/dblog/');
+    $this->assertResponse(200);

Hopefully this will fail only for views.

dagmar’s picture

Issue tags: +Novice
yogeshmpawar’s picture

Assigned: Unassigned » yogeshmpawar
yogeshmpawar’s picture

Assigned: yogeshmpawar » Unassigned
Status: Needs work » Needs review
FileSize
7.67 KB
1.86 KB

Changes addressed as per comment #36, also added an interdiff.

Status: Needs review » Needs work

The last submitted patch, 39: 2760031-39.patch, failed testing. View results

MerryHamster’s picture

Version: 8.5.x-dev » 8.6.x-dev
MerryHamster’s picture

only reroll path #39 for 8.6.x

MerryHamster’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 42: 2760031-42.patch, failed testing. View results

dagmar’s picture

Thanks for the patches. I'm afraid my comments in #36 were not addressed properly.

An assert* method should assert "something", and currently is returning a FormattableMarkup which is not ideal.

The approach then could be, take this portion of code and abstract it into a protected method

So more code should be inside the new method and should run the same assertions.

Let me know if there is something else to explain there, I will be glad to help.

yogeshmpawar’s picture

Assigned: Unassigned » yogeshmpawar
yogeshmpawar’s picture

Assigned: yogeshmpawar » Unassigned
Status: Needs work » Needs review
FileSize
7.25 KB
1.68 KB

Thanks @dagmar, As per my understanding i have updated a patch. Can you please look into the patch & tell me what i am missing or is it good to go.

dagmar’s picture

Status: Needs review » Needs work
+++ b/core/modules/dblog/tests/src/Functional/DbLogTest.php
@@ -118,6 +121,98 @@ public function testLogEventPage() {
+  /**
+   * Returns FormattableMarkup object.
+   */
+  protected function assertTranslations($text_source, $translation_variables) {
+    $this->assertNoRaw(Xss::filterAdmin(new FormattableMarkup($text_source, $translation_variables)));
+  }
+

As I mentioned in #36. I think the assertTranslations method should make this assertions:

$this->assertNoRaw($source_unfiltered);
$this->assertNoRaw($source_filtered);
$this->assertNoRaw($translation_unfiltered);
$this->assertRaw($translation_filtered);

And the assertTranslations should be ran twice, one for a single event admin/reports/dblog/1234 and other for the full list admin/reports/dblog

gawaksh’s picture

Assigned: Unassigned » gawaksh
Status: Needs work » Needs review
FileSize
6.37 KB

This Patch should solve the purpose

Status: Needs review » Needs work

The last submitted patch, 49: Log_Messages_2760031-49.patch, failed testing. View results

dagmar’s picture

Thanks @gawaksh.

I recommend you start applying the patch from #47 and make the modifications from there. You introduced some syntax errors in your patch and removed some useful function docs.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

izus’s picture

Assigned: gawaksh » Unassigned
FileSize
7.27 KB

Hi,
here is a patch simply rerolling #47 as it doesn't apply no more.

izus’s picture

Hi,
and here is a new patch based on #53 to adress #48
Thanks

Status: Needs review » Needs work

The last submitted patch, 54: 2760031-54.patch, failed testing. View results

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

anushrikumari’s picture

Status: Needs work » Needs review
FileSize
7.64 KB

Since patch #54 failed for 9.1.x so I re-rolled the patch for the same.

anushrikumari’s picture

patch after resolving the error

Status: Needs review » Needs work

The last submitted patch, 60: 2760031-56.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

anushrikumari’s picture

naresh_bavaskar’s picture

Assigned: Unassigned » naresh_bavaskar

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

sanjayk’s picture

Re-roll patch for 9.2

abhisekmazumdar’s picture

Assigned: naresh_bavaskar » Unassigned
sanjayk’s picture

FileSize
1.2 KB

Please ignore

sanjayk’s picture

Previous issues were fixed.

sanjayk’s picture

Status: Needs work » Needs review
FileSize
7.56 KB

Fixed #68 test cases.

volkswagenchick’s picture

Issue tags: +FLDC2021

Tagging for Florida DrupalCamp which is happening today, Feb 10, 12-4pm ET

Thanks

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mindbet’s picture

Status: Needs review » Reviewed & tested by the community

Tested successfully on Drupal 9.3 (see #69).

Reviewed the issue summary and reviewed the patch.

All looks correct, RTBC

larowlan’s picture

+++ b/core/lib/Drupal/Core/Logger/LogMessageParser.php
@@ -29,8 +29,8 @@ public function parseMessagePlaceholders(&$message, array &$context) {
+        // The key is now in \Drupal\Component\Utility\SafeMarkup::format() style.

nit: this needs to break at 80 chars

Other than that, this looks good to me

renatog’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.74 KB
579 bytes

Patch with fixes on #73

dagmar’s picture

Status: Needs review » Needs work

@RenatoG thanks for your patch. There is an extra file in the patch that doesnt seems related to this issue.

diff --git a/core/modules/datetime_range/datetime_range.install b/core/modules/datetime_range/datetime_range.install
new file mode 100644
index 0000000000..e69de29bb2
KapilV’s picture

Addressed #75

KapilV’s picture

Status: Needs work » Needs review
dagmar’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @KapilV patch looks good now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: 2760031-76.patch, failed testing. View results

mindbet’s picture

Status: Needs work » Reviewed & tested by the community

Setting back to RTBC.
The test failure in #79 was a glitch; re-run of test (see #76) passed.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Logger/LogMessageParser.php
    @@ -29,8 +29,9 @@ public function parseMessagePlaceholders(&$message, array &$context) {
    -      if (!empty($key) && ($key[0] === '@' || $key[0] === '%' || $key[0] === '!')) {
    -        // The key is now in \Drupal\Component\Render\FormattableMarkup style.
    +      if (!empty($key) && ($key[0] === '@' || $key[0] === '%' || $key[0] === ':')) {
    +        // The key is now in \Drupal\Component\Utility\SafeMarkup::format()
    +        // style.
    

    The change is functionally correct but the comment change is incorrect.

  2. +++ b/core/modules/dblog/src/Controller/DbLogController.php
    @@ -379,8 +380,9 @@ public function formatMessage($row) {
    +      $message = Markup::create(Xss::filterAdmin($message));
    

    Instead of doing this here we could use the render system and do

      [
        '#markup' => $message
      ]
    

    in \Drupal\dblog\Controller\DbLogController::topLogMessages() and \Drupal\dblog\Controller\DbLogController::eventDetails()

    That'd save us a Markup::create() and preserve the return type and do the admin XSS filtering consistently.

    Alternatively I think we might be able to remove it entirely as HTML in the messages apart from the

     for the backtrace is pretty meh - and auto-escape will then kick in and do what we want.
    
vsujeetkumar’s picture

Status: Needs work » Needs review
FileSize
7.95 KB
1.48 KB

Done with the changes mentioned in #81.1 ad #81.2, Please have a look and advise, if it is in the right direction.

vsujeetkumar’s picture

FileSize
7.71 KB
1.72 KB

Fixed custom command fail issue.

Status: Needs review » Needs work

The last submitted patch, 83: 2760031-83.patch, failed testing. View results

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Status: Needs work » Needs review
FileSize
7.46 KB

Rerolled #83 for 9.5.x

Cleaned up some of the file displays for this ticket also

Tried adding an interdiff but some reason it failed to generate but I did tweak DbLogController.php line 442 to use

$rows[] = [$dblog->count, $message];

vs $rows[] = [$dblog->count, ['#markup' => $message]];

The latter was preventing messages from appearing on the page not found page

Status: Needs review » Needs work

The last submitted patch, 87: 2760031-87.patch, failed testing. View results

smustgrave’s picture

Status: Needs work » Needs review
FileSize
9.38 KB
14.13 KB
smustgrave’s picture

FileSize
14.14 KB

Fixed CI failure

Anjali Rathod’s picture

Status: Needs review » Needs work
FileSize
146.41 KB
115.46 KB

Patch applied successfully, but the log messages are still not translated. attaching screenshots below.

smustgrave’s picture

Issue summary: View changes
FileSize
1.16 KB
8.01 KB

Please ignore patches #90, #89, and #87 I introduced some out of scope changes.

@Anjali Rathod thanks for testing. For next time it's not necessary to upload a screenshot of the patch applying.
Also please rename the screenshots to be more meaningful for what it's displaying.

So going back to #83 I rerolled and applied some tweaks.

This will need an IS update. Specifically
What is being fixed?
What is the proposed solution?
Step to reproduce?
before/after screenshots.
Remaining tasks.

Got it started

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Abhisheksingh27’s picture

Status: Needs work » Needs review
FileSize
7.52 KB

Adding reroll for 10.1.x.
please review

Abhisheksingh27’s picture

Reuploading the previous patch after removing unnecessary whitespaces

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs Review Queue Initiative

Issue summary is full of todos which need to be addressed

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.