Problem/Motivation

Issue is based on refactoring SafeMarkup::set() referenced in #2297703: [meta] Refactor and remove as many SafeMarkup::set() calls as possible.

/core/modules/node/node.install has a call for SafeMarkup::set() in function node_requirements. This call can be removed.

Proposed resolution

This SafeMarkup::set() can be removed and we can only use t() with a !var placeholder since the URL that is being generated is generated via system and not depending on user input.

User interface changes

N/A

API changes

N/A

CommentFileSizeAuthor
#6 remove_safemarkup_node_requirements-2451607-06.patch1.74 KBaneek
#6 interdiff-2451607-01-06.txt1.5 KBaneek
#1 remove_safemarkup_node_requirements-2451607-01.patch1.73 KBaneek

Comments

aneek’s picture

aneek commented
StatusFileSize
new remove_safemarkup_node_requirements-2451607-01.patch1.73 KB

Uploading a new patch, based on the proposed solution.

aneek’s picture

aneek commented
Assigned: Unassigned » aneek
Status: Active » Needs review
arpitr’s picture

arpitr commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +#drupalgoa2015

I tested, treating hook_requirements as function and checked the output on devel/php after installing devel module. Output seems to be same after patching.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/node/node.install
@@ -25,14 +25,14 @@ function node_requirements($phase) {
+      'description' => t('If the site is experiencing problems with permissions to content, you may have to rebuild the permissions cache. Rebuilding will remove all privileges to content and replace them with permissions based on the current modules and settings. Rebuilding may take some time if there is a lot of content or complex permission settings. After rebuilding has completed, content will automatically use the new permissions. !rebuild', array(
+        '!rebuild' => \Drupal::l(t('Rebuild permissions'), new Url('node.configure_rebuild_confirm'))
+      )),

Normally links are not done this way in translated strings since node_help() for an example... t('The <a href="!content">Content administration page</a>...

aneek’s picture

aneek commented

@alexpott, using <a href=""></a> like what you mentioned, HTML is double escaped. Generates,

If the site is experiencing problems with permissions to content, you may have to rebuild the permissions cache. Rebuilding will remove all privileges to content and replace them with permissions based on the current modules and settings. Rebuilding may take some time if there is a lot of content or complex permission settings. After rebuilding has completed, content will automatically use the new permissions. <a href="/admin/reports/status/rebuild">Rebuild permissions</a>

Code changes

diff --git a/core/modules/node/node.install b/core/modules/node/node.install
index 5f99509..1c8523d 100644
--- a/core/modules/node/node.install
+++ b/core/modules/node/node.install
@@ -31,8 +31,8 @@ function node_requirements($phase) {
       'title' => t('Node Access Permissions'),
       'value' => $value,
       // The result of t() is safe as the URL is not based on user input.
-      'description' => t('If the site is experiencing problems with permissions to content, you may have to rebuild the permissions cache. Rebuilding will remove all privileges to content and replace them with permissions based on the current modules and settings. Rebuilding may take some time if there is a lot of content or complex permission settings. After rebuilding has completed, content will automatically use the new permissions. !rebuild', array(
-        '!rebuild' => \Drupal::l(t('Rebuild permissions'), new Url('node.configure_rebuild_confirm'))
+      'description' => t('If the site is experiencing problems with permissions to content, you may have to rebuild the permissions cache. Rebuilding will remove all privileges to content and replace them with permissions based on the current modules and settings. Rebuilding may take some time if there is a lot of content or complex permission settings. After rebuilding has completed, content will automatically use the new permissions. <a href="!rebuild">Rebuild permissions</a>', array(
+        '!rebuild' => \Drupal::url('node.configure_rebuild_confirm'),
       )),
     );
   }
aneek’s picture

aneek commented
StatusFileSize
new interdiff-2451607-01-06.txt1.5 KB
new remove_safemarkup_node_requirements-2451607-06.patch1.74 KB

Never mind :-). Uploading a new patch. Please review.

aneek’s picture

aneek commented
Status: Needs work » Needs review
star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Yup, that looks better. Thanks @aneek!

star-szr’s picture

star-szr
he/him
Primary language English
commented

Minor note, can be fixed on commit if the committer agrees :)

+++ b/core/modules/node/node.install
@@ -26,14 +26,14 @@ function node_requirements($phase) {
+      // The result of t() is safe as the URL is not based on user input.

Do we even need this comment anymore? We're just doing a regular ol' t() here now, no concatenating, no SafeMarkup.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

I agree with @Cottser - removed the comment on commit. This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 9c44968 and pushed to 8.0.x. Thanks!

  • alexpott committed 9c44968 on 8.0.x 
    Issue #2451607 by aneek: Remove call to SafeMarkup::set() from...

Status: Fixed » Closed (fixed)

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

xjm’s picture

xjm
she/her
Primary language English
commented
Parent issue: #2297703: [meta] Refactor and remove as many SafeMarkup::set() calls as possible » #2280965: [meta] Remove every SafeMarkup::set() call
xjm’s picture

xjm
she/her
Primary language English
commented
Priority: Normal » Major