In the environment section of the upgrade status report, next to "Invalid permissions will trigger runtime exceptions in Drupal 10.", the list of invalid permissions contains quotes that appear to have been escaped twice so they appear as HTML entities. Example:

"perm1", "perm1" of user role: "Role Name".

This might be happening when it is run through t().

It might be easier to read if the permissions were presented in an HTML unordered list instead of comma-separated. That would remove the need for quote marks.

The code that generates this is in src/Form/UpgradeStatusForm.php.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Liam Morland created an issue. See original summary.

pmagunia made their first commit to this issue’s fork.

pmagunia’s picture

Status: Active » Needs review

Not sure this is the right way to do it.

The brackets in <li></li> were always appearing as HTML entities no matter what I did.

Feedback is welcome of course.

pmagunia’s picture

Here's a screenshot of what the output looks like with my code.

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs review » Needs work

Good find!

We know the permission names are code provided and should not have XSS, so we can also just change it to !permissions from @permissions and thus avoid the escaping. I think the quotes make it easier to identify which permission is which. Some permissions are only slightly different from others.

Liam Morland’s picture

Issue summary: View changes

If the permissions are in a comma-separated list, they should have quotes.

I think it would be even easier to read as an unordered list. That could be generated by theming it as an item list.

Gábor Hojtsy’s picture

Itemlist sounds like a good idea as well to make it readable.

pmagunia’s picture

Status: Needs work » Needs review
FileSize
104.88 KB

I was able to get permissions in quotes but wasn't able to get them in list item brackets.

Any HTML tags I add are not being rendered.

I tried using !permissions but then the substitution was not being made.

Attaching a screenshot of what my latest MR displays.

Liam Morland’s picture

All that is needed is the use of #markup. This commit removes everything except that.

Placeholders that start with ! are no longer supported.

Liam Morland’s picture

With this commit, it uses an unordered list. It wasn't working earlier because the data needs to be set as #markup.

This doesn't use a theme function, which seems heavy-handed for such a simple use case.

I have noticed that only the last role with invalid permissions is displayed. I plan to open a separate ticket to fix that once this one is done.

pmagunia’s picture

Ah I see. The markup looks much better now.

I can confirm the unordered list is being displayed correctly.

  • b8fe963 committed on 8.x-3.x
    Issue #3260723 by pmagunia, Liam Morland, Gábor Hojtsy: Quotes are...
Gábor Hojtsy’s picture

Status: Needs review » Fixed

Thanks, this looks fine, committed.

  • 05b3094 committed on 8.x-3.x
    Revert "Issue #3260723 by pmagunia, Liam Morland, Gábor Hojtsy: Quotes...
Gábor Hojtsy’s picture

Status: Fixed » Needs work

Whoops, this actually broke tests, which I noticed in unrelated issues. Tests need to be updated to still pass with the new order of information.

Gábor Hojtsy’s picture

I tried to figure out on my own how to add testing to this MR, since the project aleady has testing, but there is no apparent way. The docs also says that issues on projects with testing configured should get testing for MRs. So not sure how to make that appear. If this is an MR bug we cannot work around, we can switch to patches for this issue.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

So turns out my issue testing default configuration for DrupalCI got disabled earlier. I picked another one and that should be fine for new issues I hope, let's try moving to needs review if that triggers the testbot.

Liam Morland’s picture

Tests updated and now pass.

  • 8bd0c4c committed on 8.x-3.x
    Issue #3260723 by Liam Morland, pmagunia, Gábor Hojtsy: Quotes are...
Gábor Hojtsy’s picture

Status: Needs review » Fixed

Looks good, thanks! Committed this one!

I have noticed that only the last role with invalid permissions is displayed. I plan to open a separate ticket to fix that once this one is done.

Hah, good find! Please do!

Liam Morland’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.