Problem/Motivation

In #3250585-78: Highlight deprecated modules and themes at admin/reports/status page, providing warning and link with explanation we added dodgy t() usage as a workaround, which was only spotted after commit.

Opening this to clean it up.

Steps to reproduce

Proposed resolution

Options:

1. Make this a list outside of t() instead of a variable.

2. Can we just wrap them in \Drupal\Core\Render\Markup?

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#11 interdiff_6-11.txt1.16 KBmurilohp
#11 3259996-11.patch2 KBmurilohp
#6 3259996-6.patch950 bytesmurilohp
#6 interdiff_4-6.txt911 bytesmurilohp
#4 3259996-4.patch1.23 KBmurilohp

Comments

catch created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes

fixed link to the comment

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Related issues: +#3250585: Highlight deprecated modules and themes at admin/reports/status page, providing warning and link with explanation

Closed #3259827: Fix misuse of t() in system_requirements() as duplicate, forgot to mention that I opened this in the parent.

murilohp’s picture

murilohp commented
Status: Active » Needs review
StatusFileSize
new 3259996-4.patch1.23 KB

Hey! Maybe this patch can fix this. I tried to follow the proposed solution 2.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented 
+++ b/core/modules/system/system.install
@@ -100,10 +101,11 @@ function system_requirements($phase) {
+      $deprecated_modules_list_string = Markup::create(implode(',', $deprecated_modules_link_list));

I think this variable could just be inlined as before.

Also while we're here, there should be a space after the comma to separate multiple items.

murilohp’s picture

murilohp commented
StatusFileSize
new 3259996-6.patch950 bytes
new interdiff_4-6.txt911 bytes

Thanks for this quick response @longwave! This is a new patch addressing #5.

murilohp’s picture

murilohp commented
tmaiochi’s picture

tmaiochi commented
Assigned: Unassigned » tmaiochi

I'll review this.

tmaiochi’s picture

tmaiochi commented
Assigned: tmaiochi » Unassigned
Status: Needs review » Reviewed & tested by the community

Steps performed:
(1) Installed module
(2) Reproduced the issue.
(3) Applied patch.
(4) Code review on changes.
(5) Tested again with patch, issue resolved.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Reviewed & tested by the community » Needs work

Why not also apply this Markup-yfing to the imploding of $deprecated_themes_link_list in line 157 and $obsolete_extensions_link_list in line 171 in core/modules/system/system.install?

murilohp’s picture

murilohp commented
Status: Needs work » Needs review
StatusFileSize
new 3259996-11.patch2 KB
new interdiff_6-11.txt1.16 KB

It's a great idea! I made this new patch addressing the changes @spokje, thanks!

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs review » Reviewed & tested by the community

Thanks! RTBC for me if the TestBot returns green.

  • catch committed 23c3910 on 10.0.x 
    Issue #3259996 by murilohp, longwave, Spokje: Clean up t() usage for...

  • catch committed 593f903 on 9.4.x 
    Issue #3259996 by murilohp, longwave, Spokje: Clean up t() usage for...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.0.x and cherry-picked to 9.4.x, thanks!

Status: Fixed » Closed (fixed)

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