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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aneek’s picture

Uploading a new patch, based on the proposed solution.

aneek’s picture

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

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

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

@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

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

aneek’s picture

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

Status: Needs review » Reviewed & tested by the community

Yup, that looks better. Thanks @aneek!

star-szr’s picture

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

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’s picture

Priority: Normal » Major