Problem/Motivation

_update_message_text calls SafeMarkup::set() which is meant to be for internal use only.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because not a bug,
Issue priority Major because part of critical meta #2280965: [meta] Remove or document every SafeMarkup::set() call

Proposed resolution

  • Remove the call by refactoring the code.

Remaining tasks

  1. (done)Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123
  2. (N/A Since this is not a bug (no double escaping) I think we do not need to add tests.) Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it.

Manual testing steps (for XSS and double escaping)

N/A

User interface changes

N/A

API changes

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sclapp’s picture

sclapp’s picture

sclapp’s picture

Status: Active » Needs review
FileSize
1.94 KB

Removed string concatenation & used 2-part string with space separator and passed that through SafeMarkup::format().

sclapp’s picture

joelpittet’s picture

Status: Needs review » Needs work
+++ b/core/modules/update/update.module
--- a/example.gitignore
+++ b/example.gitignore

+++ b/example.gitignore
@@ -34,3 +34,4 @@ sites/simpletest
+*.idea*

Please move this to your global .gitignore.

Otherwise I think this is RTBC.

sclapp’s picture

Status: Needs work » Needs review
FileSize
1.71 KB

incorporated feedback from #6

joelpittet’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/update/update.module
    @@ -532,15 +532,17 @@ function _update_message_text($msg_type, $msg_reason, $report_link = FALSE, $lan
    +      $linktext = t('See the <a href="@available_updates">available updates</a> page for more information and to install your missing updates.', array('@available_updates' => \Drupal::url('update.report_update', [], ['language' => $language])), array('langcode' => $langcode));
    

    Short array syntax for the end so we don't have mixed syntax in the same line.

  2. +++ b/core/modules/update/update.module
    @@ -532,15 +532,17 @@ function _update_message_text($msg_type, $msg_reason, $report_link = FALSE, $lan
    +      $linktext = t('See the <a href="@available_updates">available updates</a> page for more information.', array('@available_updates' => \Drupal::url('update.status', [], ['language' => $language])), array('langcode' => $langcode));
    

    Same here.

  3. +++ b/core/modules/update/update.module
    @@ -532,15 +532,17 @@ function _update_message_text($msg_type, $msg_reason, $report_link = FALSE, $lan
    +    // Anything in $text has already been through t(), so it is safe.
    

    "Anything in $text has been run through t() and marked as safe."

sclapp’s picture

Status: Needs work » Needs review
FileSize
2.74 KB

fixed up syntax to be consistent for arrays & fixed up comment to be a little clearer

joelpittet’s picture

Status: Needs review » Needs work
+++ b/core/modules/update/update.module
@@ -517,10 +517,10 @@ function _update_message_text($msg_type, $msg_reason, $report_link = FALSE, $lan
-        $text = t('There was a problem checking <a href="@update-report">available updates</a> for Drupal.', array('@update-report' => \Drupal::url('update.status')), array('langcode' => $langcode));
+        $text = t('There was a problem checking <a href="@update-report">available updates</a> for Drupal.', ['@update-report' => \Drupal::url('update.status')], ['langcode' => $langcode]);
...
-        $text = t('There was a problem checking <a href="@update-report">available updates</a> for your modules or themes.', array('@update-report' => \Drupal::url('update.status')), array('langcode' => $langcode));
+        $text = t('There was a problem checking <a href="@update-report">available updates</a> for your modules or themes.', ['@update-report' => \Drupal::url('update.status')], ['langcode' => $langcode]);

Whoops sorry, these array syntax changes are out of scope of this patch because these lines weren't needing change. The array syntax change should only change on the lines you were working on. Sorry if I miss lead you on that instruction.

The reason is someone in another patch may be working on those and if this patch got committed they would have to re-roll their patch. Or that's the theory.

All the other changes are great and are RTBC.

sclapp’s picture

Status: Needs work » Needs review
FileSize
1.7 KB

hopefully, just removed the out of scope changes to syntax while including earlier patch code

joelpittet’s picture

Status: Needs review » Needs work

I should be a bit more thorough:( Sorry for keep sending this back.

  1. +++ b/core/modules/update/update.module
    @@ -532,15 +532,17 @@ function _update_message_text($msg_type, $msg_reason, $report_link = FALSE, $lan
    +      $linktext = t('See the <a href="@available_updates">available updates</a> page for more information and to install your missing updates.', ['@available_updates' => \Drupal::url('update.report_update', [], ['language' => $language])], ['langcode' => $langcode]);
    ...
    +      $linktext = t('See the <a href="@available_updates">available updates</a> page for more information.', ['@available_updates' => \Drupal::url('update.status', [], ['language' => $language])], ['langcode' => $langcode]);
    

    The $linktext should separate the words with an underscore. like $link_text. Maybe there is a better variable name for this? $additional_text? Or $available_updates_text?

  2. +++ b/core/modules/update/update.module
    @@ -532,15 +532,17 @@ function _update_message_text($msg_type, $msg_reason, $report_link = FALSE, $lan
    +      return SafeMarkup::format('@text @linktext', ['@text' => $text, '@linktext' => $linktext]);
    +    }
    

    These need indenting back.

  3. +++ b/core/modules/update/update.module
    @@ -532,15 +532,17 @@ function _update_message_text($msg_type, $msg_reason, $report_link = FALSE, $lan
    +      else {
    ...
       }
    

    These lines are not needed.

  4. +++ b/core/modules/update/update.module
    @@ -532,15 +532,17 @@ function _update_message_text($msg_type, $msg_reason, $report_link = FALSE, $lan
    +      // Anything in $text has been run through t() and marked as safe.
    +      return $text;
    ...
    -  // All strings are t() and empty space concatenated so return SafeMarkup.
    -  return SafeMarkup::set($text);
    

    These two lines should be indented on the same line. (so back 2 spaces for the new one)

kgoel’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.78 KB
1.84 KB
75.65 KB

1. I have changed the variable from $linktext to $available_updates_text.
2. fixed
3. fixed
4. fixed

I have not changed the array syntax to make it short array syntax but made it consistent with other array in the _update_message_text function.

YesCT’s picture

Status: Needs review » Needs work

thanks for updating that.

yeah, so we can use git diff --color-words, I think it is ok to not change the array syntax.

+++ b/core/modules/update/update.module
@@ -532,15 +532,17 @@ function _update_message_text($msg_type, $msg_reason, $report_link = FALSE, $lan
+    return SafeMarkup::format('@text @available_updates_text', ['@text' => $text, '@available_updates_text' => $available_updates_text]);
+    }
+  else {

missed one of the alignments.

kgoel’s picture

Status: Needs work » Needs review
FileSize
1.78 KB
802 bytes

Thank you for review!

It's so embarrassing that I missed one of the indentation :(

YesCT’s picture

I'm not sure which lines @joelpittet said where not needed in #12 3.

joelpittet’s picture

+++ b/core/modules/update/update.module
@@ -532,15 +532,17 @@ function _update_message_text($msg_type, $msg_reason, $report_link = FALSE, $lan
+  else {
+    // Anything in $text has been run through t() and marked as safe.
+    return $text;
   }
-
-  // All strings are t() and empty space concatenated so return SafeMarkup.
-  return SafeMarkup::set($text);

So what I was trying to say was that everything that remains in this function at the end is to be returned so the else itself is not needed.

Just make the hunk read:

// Anything in $text has been run through t() and marked as safe.
return $text;

Without the else.

kgoel’s picture

FileSize
1.76 KB
649 bytes
YesCT’s picture

Assigned: sclapp » Unassigned
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
128.05 KB

from the issue summary template (for safemarkup issues)

Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it.

Since this is not a bug (no double escaping) I think we do not need to add tests.

Manually tested, looking at

markup

  <td>
          <a href="/drupal2/admin/reports/updates/update">Unknown release date (version 8.0.0-beta11 available)</a>
                      <div class="description">There was a problem checking <a href="/drupal2/admin/reports/updates">available updates</a> for Drupal. See the <a href="/drupal2/admin/reports/updates">available updates</a> page for more information.</div>
                  </td>

or

<td>
          <a href="/drupal2/admin/reports/updates/update">Unknown release date (version 8.0.0-beta11 available)</a>
                      <div class="description">There was a problem checking <a href="/drupal2/admin/reports/updates">available updates</a> for Drupal. See the <a href="/drupal2/admin/reports/updates/update">available updates</a> page for more information and to install your missing updates.</div>
                  </td>

are both the same in head and with the patch.

screenshot also shows this still looks ok (the same)

YesCT’s picture

Issue summary: View changes
kgoel’s picture

Issue summary: View changes

Added beta evaluations

xjm’s picture

Assigned: Unassigned » xjm

Assigning to myself for additional feedback, but leaving at RTBC for now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 2501683-18.patch, failed testing.

davidhernandez’s picture

Issue tags: +Needs reroll

This was RTBC so I assume it just needs a reroll?

izus’s picture

Status: Needs work » Needs review
FileSize
1.76 KB

And here is the reroll

lauriii’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

Everything seems to work on my manual tests. Settings this back to RTBC for xjm's review.

xjm’s picture

Assigned: xjm » alexpott

Discussed with @alexpott; he has a suggestion for this one.

alexpott’s picture

It's a shame that _update_message_text can't return a render array because it is being used in hook_requirements as this would allow us to pass back [#markup => $text] and then renderPlain in the email use case.

That said we do have #2505499: Explicitly support render arrays in hook_requirements() open.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

NR to see if we can implement part of #28?

dawehner’s picture

What about getting #25 in and adding a todo to #2505499: Explicitly support render arrays in hook_requirements() ?

lauriii’s picture

I don't think #2505499: Explicitly support render arrays in hook_requirements() is necessary in order to get this fixed. That issue also could be worked on later to get it in for 8.1.x so +1 for @dawehners suggestions.

YesCT’s picture

Assigned: alexpott » Unassigned

I dont think we are waiting on anything else from @alexpott here. and we have somethings someone could try and do. so unassigning.

lauriii queued 25: 2501683-25.patch for re-testing.

kgoel’s picture

Patch from #25 still applies. Anyone up for review the patch and help move this issue forward?

YesCT’s picture

I'm going to work on this now.

YesCT’s picture

added the @todo.

[edit. oops. wrong patch file. new one coming]

YesCT’s picture

re-verified that the markup on /admin/reports/status is still fine.

YesCT’s picture

interdiff from 25.36 was good, but here is a patch with all the changes.

YesCT’s picture

Issue summary: View changes

updating the reason for no test changes.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Code looks good and visually it has remained same so this is RTBC for me. Making hook_requirements support render arrays is nice thing to do but doesn't fit the scope of this issue so its nice it gets done in follow-up. We possibly can even do that without BC break.

xjm’s picture

Assigned: Unassigned » xjm
Status: Reviewed & tested by the community » Needs work

Discussed with @alexpott -- I'm going to tweak the documentation on this.

xjm’s picture

Status: Needs work » Needs review
FileSize
4.15 KB
4.75 KB

Actually, in the process of tweaking this, I finally remembered what @alexpott and I discussed a month or two back (and never wrote down, doh!).

As stated in the documentation for _update_message_text(), the output is used in two places: in the Update module's hook_requirements() implementation, and in its hook_mail() implementation. Here are those two usages.

In _update_requirement_check() which in turn is used only in update_mail():

  if ($status != UPDATE_CURRENT) {
    $requirement ['reason'] = $status;
    $requirement ['description'] = _update_message_text($type, $status, TRUE);
    $requirement ['severity'] = REQUIREMENT_ERROR;
  }

Then in update_mail():

  foreach ($params as $msg_type => $msg_reason) {
    $message ['body'][] = _update_message_text($msg_type, $msg_reason, FALSE, $langcode);
  }

Note that in the first case, the $report_link parameter is always true and the $langcode parameter is always empty, whereas in the second case, $report_link is always false. Now look at this hunk of code at the end of _update_message_text():

  if (!empty($langcode)) {
    $language = \Drupal::languageManager()->getLanguage($langcode);
  }
  else {
    $language = NULL;
  }
  if ($report_link) {
    if (update_manager_access()) {
      $text .= ' ' . t('See the <a href="@available_updates">available updates</a> page for more information and to install your missing updates.', array('@available_updates' => \Drupal::url('update.report_update', [], ['language' => $language])), array('langcode' => $langcode));
    }
    else {
      $text .= ' ' . t('See the <a href="@available_updates">available updates</a> page for more information.', array('@available_updates' => \Drupal::url('update.status', [], ['language' => $language])), array('langcode' => $langcode));
    }
  }

If $report_link is true, $langcode and therefore $language will always be null given the usages above! So the use of both here doesn't make any sense. Furthermore, since the link to the available updates page is only used in the helper function for update_requirements(), there's no reason to have the unimplemented, untested code path for it for update_mail().

Therefore, the attached patch adds the report link in _update_requirement_check() instead, removes the unneeded language checking for that link, removes the need for the extra boolean flag parameter altogether, and makes _update_message_text() return a simple single translated string based on the switch logic.

Hopefully I haven't missed anything. I also personally think we should deal with the hook_requirements() render array thing since it's only a small bit of documentation, but let's make sure this is sane first.

I think the signature and return value change for _update_message_text() is appropriate and does not require a CR or anything because it is prefixed by a _ (thus "internal") and we're essentially removing a dead code path that unnecessarily complicates the assembly of this string.

xjm’s picture

+++ b/core/modules/update/update.install
@@ -107,8 +107,20 @@ function _update_requirement_check($project, $type) {
+    ;

Nit: This line of code does nothing. ;)

xjm’s picture

Status: Needs work » Needs review
FileSize
4.56 KB
1.23 KB

Also another silly mistake.

xjm’s picture

Manually tested using the testing in #19 (thanks @YesCT) and confirmed it still looks the same. Aside: an out-of-scope usability/accessibility bug is that we have the words "available updates" linking to two different pages in the same paragraph...! You can see this in the screenshot in #19.

Here is the interdiff that, applied with #43, simply uses a render array instead. Note that this already works--we already support render arrays for hook_requirements(). It's just not documented as such. I tested manually in exactly the same way and confirmed this.

So, my preference would be to just use that now, without blocking it on #2505499: Explicitly support render arrays in hook_requirements() which should just be a docs patch.

xjm’s picture

Issue tags: +Needs followup

For the link text.

YesCT’s picture

I'm going to review this now, and take care of the follow-up.

YesCT’s picture

+++ b/core/modules/update/update.install
@@ -107,8 +107,20 @@ function _update_requirement_check($project, $type) {
+    // @todo Evaluate whether to use a render array instead following
+    //  https://www.drupal.org/node/2505499.

so @xjm found out #2505499: Explicitly support render arrays in hook_requirements() is actually postponed and wont just be a docs patch.

YesCT’s picture

Issue tags: -Needs followup
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed all changes and code-paths in this change to ensure anything wasn't missed. +1 it's nice little clean-up even with the @todo regarding SafeMarkup::format vs render array.

+++ b/core/modules/update/update.install
@@ -107,8 +108,19 @@ function _update_requirement_check($project, $type) {
+    $requirement['description'] =  SafeMarkup::format('@text @available_updates_text', ['@text' => _update_message_text($type, $status), '@available_updates_text' => $available_updates_text]);

There is a double space after the = that could be cleaned up on commit.

stefan.r’s picture

re #49, looks like this is going ahead after all (see #2505499: Explicitly support render arrays in hook_requirements())

So should this use a render array after all?

alexpott’s picture

Title: Remove or document SafeMarkup::set in _update_message_text » Remove SafeMarkup::set in _update_message_text
xjm’s picture

Assigned: xjm » Unassigned
Status: Reviewed & tested by the community » Needs work

Yep, let's update this to be a render array. Thanks @stefan.r!

stefan.r’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing
FileSize
2.16 KB
4.21 KB

see #19

josephdpurcell’s picture

Reviewing and doing manual testing.

josephdpurcell’s picture

Status: Needs review » Needs work
FileSize
211.42 KB

Initial manual test shows there is no space between sentences, see screenshot:

josephdpurcell’s picture

Status: Needs work » Needs review
FileSize
4.24 KB
1.38 KB
+++ b/core/modules/update/update.module
@@ -524,23 +520,8 @@ function _update_message_text($msg_type, $msg_reason, $report_link = FALSE, $lan
-      $text .= ' ' . t('See the <a href="@available_updates">available updates</a> page for more information and to install your missing updates.', array('@available_updates' => \Drupal::url('update.report_update', [], ['language' => $language])), array('langcode' => $langcode));
...
-      $text .= ' ' . t('See the <a href="@available_updates">available updates</a> page for more information.', array('@available_updates' => \Drupal::url('update.status', [], ['language' => $language])), array('langcode' => $langcode));

These spaces were removed causing the issue I noticed where there is no space between sentences.

This patch will fix it by adding those spaces back in as a #prefix. Use #prefix to avoid unnecessarily needing to escape and filter that space.

josephdpurcell’s picture

Issue summary: View changes
FileSize
195.14 KB

Manually checking this, here is the screenshot:

Also, I removed the screenshot from the description since I assume that was put there by accident.

dsnopek’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

I manually tested @josephdpurcell's patch from #58. There was no change in the message on the "Status report" page from current core (so it matches the screenshot from #58 and has the space between the two sentences).

Since this was RTBC before it was marked as "Needs manual testing" (in #51), I'm setting back to RTBC!

If the testbot agrees, this might be ready. :-)

alexpott’s picture

Should we still wait for #2505499: Explicitly support render arrays in hook_requirements()? Maybe as this code only fires in the runtime requirements checks maybe it's ok to go now?

xjm’s picture

@alexpott I don't think it needs to wait since it's the runtime phase only.

xjm’s picture

Title: Remove SafeMarkup::set in _update_message_text » Remove SafeMarkup::set in _update_message_text()
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

@xjm I agree.

Committed 8ba635b and pushed to 8.0.x. Thanks!

  • alexpott committed 8ba635b on 8.0.x
    Issue #2501683 by sclapp, kgoel, xjm, YesCT, josephdpurcell, stefan.r,...

Status: Fixed » Closed (fixed)

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