Remove system_requirements() SafeMarkup::set() use

I wonder if this still needs a change as SafeMarkup::implode() does not seem to exist any more.

@arpitr, no the function name you mentioned is not available now. So we have to re-create a new patch.

Uploading a new patch. Review!!

I think we should update the issue as well as the solution does not go inline to what was proposed.

Just a suggestion: Would it be possible if we're going with inline template to actually construct the description in the inline template, in other words do all the concatenating and such inside the #template?

1. +++ b/core/modules/system/system.install
   @@ -348,11 +348,14 @@ function system_requirements($phase) {
   +      //The $description string is concatenated from t() calls,
   +      //safe drupal_render() output, whitespace, and <br /> tags, so is safe.
   +      //We can implement raw filter in the inline_template call.
   

   Coding standards: spaces needed after // ref. https://www.drupal.org/node/1354#inline

2. +++ b/core/modules/system/system.install
   @@ -348,11 +348,14 @@ function system_requirements($phase) {
   +        '#template' => '{{ description|raw}}',
   

   Twig coding standards: Missing space inside closing curly braces. Ref. http://twig.sensiolabs.org/doc/coding_standards.html (Drupal's Twig coding standards are at https://www.drupal.org/node/1823416)

Fixed the standards

My bad @Cottser. Thanks for the proper review. And thanks @lauriii for saving me some time ;-) ha ha... Making this RTBC to get alex's review.

@aneek - it's not customary to rtbc patches you've worked on - unless the patch has completely changed or was rtbc for very very minor nits.

@alexpott - noted. Well then someone else needs to set this as RTBC if review is fine.

Hello,

I noticed that the output in drush rq is wrong:

Cron maintenance tasks Info Last run 10 min 20 sec ago
Array

Drush expects a plain string and gets the inline template for the description and therefore prints it as "Array".
Attaching a patch that fixes this + fixes some spaces issues after/before sentences in drush rq as it strips tags.

Not sure what happened here with the testbot O_o

@herved awesome!! But can you post an interdiff?
Thanks!

Sure here it is.

1. +++ b/core/modules/system/system.install
   @@ -338,24 +338,25 @@ function system_requirements($phase) {
   +    // The $description string is concatenated from t() calls, safe
   +    // Renderer::render() output, whitespace, and <br /> tags, so is safe.
   +    // We can implement raw filter in the inline_template call.
   

   $description actually doesn't contain any Renderer::render() output at this point.

2. +++ b/core/modules/system/system.install
   @@ -338,24 +338,25 @@ function system_requirements($phase) {
   +      'description' => \Drupal::service('renderer')->render($description),
   

   I've tested it and it works fine when passing the $description render array directly.

   If this doesn't work with drush rq then this is actually a bug in drush.

   I would put $description here.

Reported the issue in Drush: Issue #1314: drush core-requirements does not output descriptions in render arrays.

I'm not sure about the applicability of the patch, but this issue needs a summary update, as well as a beta evaluation.

Here's some things to consider:

1. +++ b/core/modules/system/system.install
   @@ -338,21 +338,25 @@ function system_requirements($phase) {
        $description = '';
   ...
   +      $description = t('Cron has not run recently.') . ' ' . $help . ' ';
   ...
   +    $description .= t('You can <a href="@cron">run cron manually</a>.', array('@cron' => \Drupal::url('system.run_cron'))) . ' ';
   ...
   +    $description = array(
   ...
   +      'description' => \Drupal::service('renderer')->render($description),
   

   Not a fan of re-using the variable.

2. +++ b/core/modules/system/system.install
   @@ -338,21 +338,25 @@ function system_requirements($phase) {
        $description .= '<br />' . t('To run cron from outside the site, go to <a href="!cron">!cron</a>', array('!cron' => \Drupal::url('system.cron', array('key' => \Drupal::state()->get('system.cron_key'), array('absolute' => TRUE)))));
   

   This might be a little out of scope, but this line needs array reformatting for coding standards. Pull it out to multiple lines. https://www.drupal.org/coding-standards#array

3. +++ b/core/modules/system/system.install
   @@ -338,21 +338,25 @@ function system_requirements($phase) {
   +    // The $description string is concatenated from t() calls, safe
   +    // Renderer::render() output, whitespace, and <br /> tags, so is safe.
   +    // We can implement raw filter in the inline_template call.
   

   Wrap on 80. The comments also need some punctuation.

Pulled long line into multiple lines and actually found a bug.
The 3rd argument of the call to Drupal::url() ('absolute' => TRUE) never reached the method call because of a parenthesis mixup.

Removed comment regarding using the Renderer::render() method since it is not applicable.

Old patches failed applying, created a new patch with same changes.

(There was a period added in 8.0.x in one of the lines that is removed by this patch causing it to fail applying)

Setting to needs review so it gets tested. Thanks!

Updated the issue summary to reflect the @todo comment I found in the system.install file line 366 (see below)

Did some more reading on the issue (this thread and parent issue etc) and made some additional changes:

- constructed the description 'in' the inline template as suggested by @Cottser comment 11 moving the use of the <br /> into the template.
- removed the reuse $description variable as pointed out by @Mile23 comment 25 sub 1
- removed the call to \Drupal::service('renderer')->render($description) as it is unnecessary as pointed out by @pfrenssen in comment 23 sub 2

So far I think this patch solves the issue present in the file system.install line 366

      // @todo This string is concatenated from t() calls, safe drupal_render()
      //   output, whitespace, and <br /> tags, so is safe. However, as a best
      //   practice, we should not use SafeMarkup::set() around a variable. Fix
      //   in: https://www.drupal.org/node/2296929.
      'description' => SafeMarkup::set($description),

Btw, can someone please point out where/how I can see this code in action? (I am new to the drupal codebase)

Now we're using the inline template we can get rid of all of the creating t'd strings in PHP variables - which has the massive advantage of actually putting all the description test in one place.

@codemonkie you can see this text on the url admin/reports/status

One of the spaces was not in the logically correct place - moved. This actually made no difference to how it looks on screen but it makes more sense.

I agree that we probably should keep this particular translatable as 2 separate strings, at least for the scope of this issue.

As for how the trans tag is scanned, there is #2040089: Support for Drupal 8 twig %trans% template translatables but I'm not sure if that handles inline templates or not.

Things like ifs, trans blocks etc. shouldn't have a problem being on their own line. If need be whitespace modifiers can be used to trim any additional whitespace.

By splitting up the template onto multiple lines we not only have easier to read code but we also have better odds at complying with Twig coding standards, upstream and our own.

Edit: And I'm not sure yet how I feel about widespread use of this pattern :)

Another consideration is non-HTML output of this string. What happens with Drush now? See #17.

See #2280965-20: [meta] Remove every SafeMarkup::set() call for my thoughts on SafeMarkup::format() vs. inline_template in general.

Regardless of that though, I suggest especially not using inline_template, or any other render array, or invoking drupal_render(), within hook_requirements() implementations. Seems like an unnecessary and potentially problematic dependency for use cases that want to check requirements outside of the rendering pipeline.

Agreed. Also a lot of the problem with the existing code is that it goes against the translatability guidelines at https://www.drupal.org/node/322774 (at least the way I read them).

Fix that, and then the whole thing basically just becomes SafeMarkup::format('@line_one<br />@line_two') which is relatively simple. See attached.

I don't get it to be honest. If we shouldn't use all of drupal's toolset here and if we care about cli output why are we concatenating html in the first place?

Also the use of inline template should actually result in less happening when something external to the render pipeline invokes hook requirements.

Yes the fact that there's HTML kind of messes up some of these assumptions unless we are also assuming that a CLI is going to strip tags and maybe "br2nl". If we are actually caring about CLIs or others then (out of scope) maybe we need to provide plain text versions of requirements?

The fact that there is logic needed to display this string (HTML or not) to me steers towards an (inline) template-based implementation. What if third or fourth lines are added to this string later? The inline template can scale much better to accommodate that versus a big concatenation.

In other words…

+++ b/core/modules/system/system.install
@@ -351,23 +349,20 @@ function system_requirements($phase) {
+    $description = SafeMarkup::format('@line_one<br />@line_two', array('@line_one' => $line_one, '@line_two' => $line_two));

This just seems unfortunate.

The fact that there is logic needed to display this string (HTML or not) to me steers towards an (inline) template-based implementation.

I agree that in general, if statements that are purely front-end logic are better in Twig than in PHP. That might be a good guideline to explicitly add to https://www.drupal.org/node/2311123.

What I wonder though is if that is better enough to require callers of hook_requirements() outside of Drupal's own Web UI to have to know that 'description' can be a render array and not just a string. Maybe yes, though we should recognize that that is an API change for the @return of that hook.

Yes the fact that there's HTML kind of messes up some of these assumptions unless we are also assuming that a CLI is going to strip tags and maybe "br2nl".

The difference is that if a tool like Drush outputs an HTML string to a non-HTML agent, the meaning of the message is still clear to the person reading it. Array is not.

I suppose for completeness, we should also consider if this is the kind of use case that option 3 (calling the Twig service directly) is best for?

In other words…
...

+    $description = SafeMarkup::format('@line_one<br />@line_two', array('@line_one' => $line_one, '@line_two' => $line_two));

This just seems unfortunate.

I don't think there's any completely pretty solution here except the one that's already in place - SafeMarkup::set() :)

Which, with the other suggested refactoring, would actually look something this:

  if ($severity == REQUIREMENT_INFO) {
    $description = t('You can run cron manually....');
  }
  else {
    $description = t('Cron has not run recently. For more information....');
  }
  $description = SafeMarkup::set($description . '<br />' . t('To run cron from outside the site....'));

To be honest, I'm not sure there's anything wrong with that pattern? It's pretty clear from looking at that code why the concatenated string is known to be safe. I get that we don't want to encourage developers to decide on their own if something is safe... but maybe it's OK for really simple situations?

Or another option, just put the <br /> inside a single t() call (along with the sentences before and after it) and be done.

The sentences are all related, so it's basically just a single paragraph anyway. It would be a little bit of duplicate work for translators to separately translate the two versions of that paragraph, but really not much.

Maybe yes, though we should recognize that that is an API change for the @return of that hook.

As an example, drupal_check_module() calls $message = SafeMarkup::escape($requirement['description']);, which would fail if $requirement['description'] is an array.

But, looks like HEAD already has system_requirements() setting some other 'description's to 'inline_template' arrays. So this issue wouldn't introduce the problem, only add to it.

However, I checked all of HEAD's other (not system_requirements()) usages of 'inline_template' and they're ok in terms of only being used within a place where render arrays are expected. So my preference would be to explicitly decide here what we want as the @return of hook_requirements() and either fix the existing deviations or the existing callers as needed.

And it seems to me that we already have an assumption Drupal 8 at this point that returning a render array should be equivalent to/interchangeable with returning a string of HTML.

I don't think we have that assumption at all. For example, controllers, BlockInterface::build(), FormatterInterface::viewElements(), and many other similar methods must return render arrays; returning an HTML string would fail. Similarly, children of render arrays must also be render arrays, not strings. I think the only place they're interchangeable is for render array properties (such as #description) and variables prepared in preprocess functions.

I also don't think we have any services that return render arrays, though I might be wrong on that. Though that's correlated with services also generally not returning HTML.

That aside, I think it's definitely legit to have HTML in system_requirements() messages (and other messages). Not only whitespace but also things like lists, links, etc.

Currently, hook_help() can have all those things, but the @return is a string, not a render array. What other message-like return values should we consider if we decide to make the API change to hook_requirements()[]['description']?

Moved on in the direction @alexpott was going in #34 as there seemed to be a little more support for that and did the following work

• rerolled patch
• (re)separated strings for translators as pointed out by @xjm #35, @Cottser #36
• broke Twig template up into multiple lines as suggested by @xjm #35 and tidied the resulting whitespaces

To break the inline template into multiple lines you end up with somewhat of a whitespace war. This is because whitespace at the end of a line is not accepted and this is where you would typically put the whitespace. I ended up with a method where I explicitly defined a whitespace as a variable space. Further I stripped the whitespace due to indentation with the '-' modifier. This results into nicely formatted output, so no extra whitespace whatsoever.

Not sure if the whitespace variable is the way to go though. It does end up in a more readable template with nice output. I would like to hear more opinions on this.

You removed the <br /> entirely. Not sure if that was intentional (it was actually a suggestion in #46) but if so there is definitely no need for a Twig template at all... It's just a single translatable string with two variations.

Any thoughts on that (or also #44, just put the <br /> inside t())?

It seems really weird to me to have a whole inline Twig template for something this simple. There is way more code logic inside that template than anything HTML-structure-related. I get that it might be different in the general case, although hopefully not too many people are putting long paragraphs or bulleted lists in what's intended to be a short description.

@David_Rothstein, indeed, I forgot to mention the removal of the <br /> tag (as suggested in #46 by @xjm).
I added some before-after screenshots for a comparison of the usability impact.

I agree with you that the use of a an inline template is like killing a fly with a hammer in this case, but maybe this is more of an example case.

I do see some advantages:

• All strings are in one place, so easier to scan, follow, make sense of as a whole.
• All variables needed are in one place, supplied as context, making them easier to find.
• Besides the <br /> there is also a conditional string, of which the render logic is now clearly visible.

and some disadvantages:

• More overhead, verbose, more code means more chance of errors.
• Harder to understand if not familiar with the inline template concept.
• Whitespace control is somewhat of a hassle.

@David_Rothstein, about combining the strings (#44), this results in new strings to be translated, so more work for translators. I'm not sure what the impact is of that.
I do think it is actually a better solution, because it gives the translators more control where and how to use the variables. The opinion of an experienced Drupal translator would help in this.

Thanks for the screenshots. I think we would also want to see how it looks for the other condition. And someone asked also for testing with drush.

The inline template ... looks like too much.
I'm going to try using format again, keeping the logic and the translatable strings as is.

No interdiff since it is a different approach.

With this approach:

• Each t'd string is individually marked safe, and then only the aggregated string is. No intermediate partial aggregation strings are marked safe.
• The logic is staying the same.
• The br tag remains (so no UI change).

======

Tested with the conditional string by adding $severity = REQUIREMENT_WARNING; before the if

    if ($severity != REQUIREMENT_INFO) {

I added more context to the screenshots (mostly so I would remember that this is the status report page, and not the cron form, which confused me for a bit).

severity: warning

head

markup

        <td>
          Last run 1 hour 22 minutes ago
                      <div class="description">Cron has not run recently. For more information, see the online handbook entry for <a href="https://www.drupal.org/cron">configuring cron jobs</a>. You can <a href="/drupal/admin/reports/status/run-cron?token=-2p6BCx63owpsVyZ19SDp8C1nVIt0wSRU2qvgIvLkn0">run cron manually</a>.<br />To run cron from outside the site, go to <a href="/drupal/cron/6_P1AmCzM4u9__Jo-i5K3rhckWBOxCliZ4HQCIlUiRyAsSliaUwtegqYk7OiNlE6Ubj0A1z9aw?0%5Babsolute%5D=1">/drupal/cron/6_P1AmCzM4u9__Jo-i5K3rhckWBOxCliZ4HQCIlUiRyAsSliaUwtegqYk7OiNlE6Ubj0A1z9aw?0%5Babsolute%5D=1</a></div>
                  </td>

patch

markup

        <td>
          Last run 1 hour 23 minutes ago
                      <div class="description">Cron has not run recently. For more information, see the online handbook entry for <a href="https://www.drupal.org/cron">configuring cron jobs</a>. You can <a href="/drupal2/admin/reports/status/run-cron?token=KoZQTg_b8J9de_AA6HPv2pW7MuCq8MM4JGoGInzaBKE">run cron manually</a>.<br />To run cron from outside the site, go to <a href="/drupal2/cron/Te4FsQTQJHggRz18AXwWh7Zu8v5GSjDYeJ3tt8Unj1PIr2-OeNGb3xHGhTVno8mzeDSYFbCMtw?0%5Babsolute%5D=1">/drupal2/cron/Te4FsQTQJHggRz18AXwWh7Zu8v5GSjDYeJ3tt8Unj1PIr2-OeNGb3xHGhTVno8mzeDSYFbCMtw?0%5Babsolute%5D=1</a></div>
                  </td>

severity: info

head

markup

        <td>
          Last run 1 hour 13 minutes ago
                      <div class="description"> You can <a href="/drupal/admin/reports/status/run-cron?token=-2p6BCx63owpsVyZ19SDp8C1nVIt0wSRU2qvgIvLkn0">run cron manually</a>.<br />To run cron from outside the site, go to <a href="/drupal/cron/6_P1AmCzM4u9__Jo-i5K3rhckWBOxCliZ4HQCIlUiRyAsSliaUwtegqYk7OiNlE6Ubj0A1z9aw?0%5Babsolute%5D=1">/drupal/cron/6_P1AmCzM4u9__Jo-i5K3rhckWBOxCliZ4HQCIlUiRyAsSliaUwtegqYk7OiNlE6Ubj0A1z9aw?0%5Babsolute%5D=1</a></div>
                  </td>

patch

markup

        <td>
          Last run 1 hour 15 minutes ago
                      <div class="description">You can <a href="/drupal2/admin/reports/status/run-cron?token=KoZQTg_b8J9de_AA6HPv2pW7MuCq8MM4JGoGInzaBKE">run cron manually</a>.<br />To run cron from outside the site, go to <a href="/drupal2/cron/Te4FsQTQJHggRz18AXwWh7Zu8v5GSjDYeJ3tt8Unj1PIr2-OeNGb3xHGhTVno8mzeDSYFbCMtw?0%5Babsolute%5D=1">/drupal2/cron/Te4FsQTQJHggRz18AXwWh7Zu8v5GSjDYeJ3tt8Unj1PIr2-OeNGb3xHGhTVno8mzeDSYFbCMtw?0%5Babsolute%5D=1</a></div>
                  </td>

=====
Note there is a difference in the markup for the case where severity is info, in head, there is a leading space due to, in head:

    $description .= ' ' . t('You can <a href="@cron">run cron manually</a>.', array('@cron' => \Drupal::url('system.run_cron')));

(No difference in markup for the severity warning case.)

@herved thanks for noting the drush command in #17

I also tested with drush rq (drush requirements):
in head

 Cron maintenance       Warning   Last run 1 hour 11 minutes ago
 tasks                            Cron has not run recently. For more information, see the online handbook entry
                                  for configuring cron jobs. You can run cron manually.To run cron from outside
                                  the site, go to
                                  /cron/6_P1AmCzM4u9__Jo-i5K3rhckWBOxCliZ4HQCIlUiRyAsSliaUwtegqYk7OiNlE6Ubj0A1z9a
                                  w?0%5Babsolute%5D=1

with the patch

 Cron maintenance       Warning   Last run 1 hour 11 minutes ago
 tasks                            Cron has not run recently. For more information, see the online handbook entry for configuring cron jobs. You can run
                                  cron manually.To run cron from outside the site, go to
                                  /cron/Te4FsQTQJHggRz18AXwWh7Zu8v5GSjDYeJ3tt8Unj1PIr2-OeNGb3xHGhTVno8mzeDSYFbCMtw?0%5Babsolute%5D=1

[edit: php tag to show the html]

Now with patch.

1. +++ b/core/modules/system/system.install
   @@ -360,23 +360,31 @@ function system_requirements($phase) {
   +    $description_string = '';
   +    $description_replacements = [];
   

   It would be nice to comment that you are using these variables in the safeMarkup::format.
   

2. +++ b/core/modules/system/system.install
   @@ -360,23 +360,31 @@ function system_requirements($phase) {
   +    $description_string = '';
   +    $description_replacements = [];
   

   For better readability of the code, lets flip the order of variables.

3. +++ b/core/modules/system/system.install
   @@ -360,23 +360,31 @@ function system_requirements($phase) {
   +    $description_string .= '@description_manual<br />@description_additional_info';
   +    $description_replacements += [
   +      '@description_manual' => $description_manual,
   +      '@description_additional_info' => $description_additional_info,
   +    ];
   

   Let's flip the order of $description_string and $description_replacements to be consistent.

address both of those points.

+    $description_string .= '@description_manual<br />@description_additional_info';

When I tried that above, the reactions included words like "unfortunate" and "yeach"...

I think these placeholder names are at least better than mine, though - mine were "@line_one" and "@line_two" :)

@David_Rothstein, about combining the strings (#44), this results in new strings to be translated, so more work for translators. I'm not sure what the impact is of that.
I do think it is actually a better solution, because it gives the translators more control where and how to use the variables. The opinion of an experienced Drupal translator would help in this.

I agree. I was basing what I said on https://www.drupal.org/node/322774 (which basically implies that if a whole paragraph is translatable, it should all go inside the same t() call) but hearing from someone who actually does translations (which is not me) would be good.

ok. I'll try doing 2. now.

ok, this does 2.

with warning severity, markup:

          Last run 1 hour 38 minutes ago
                      <div class="description">Cron has not run recently. For more information, see the online handbook entry for <a href="https://www.drupal.org/cron">configuring cron jobs</a>. You can <a href="/drupal2/admin/reports/status/run-cron?token=8bQlufrbqSlG7jjBUwax2BKNXUI9PmLiV9iEG0zRbWk">run cron manually</a>.<br />To run cron from outside the site, go to <a href="/drupal2/cron/7Ws6Z_imhMrIUhb_lAcZq8UNA-1VRFIvMl8vbMbElrt5UCgcp-CRR2JfiTxpPTyi0n4YDRlLUA?0%5Babsolute%5D=1">/drupal2/cron/7Ws6Z_imhMrIUhb_lAcZq8UNA-1VRFIvMl8vbMbElrt5UCgcp-CRR2JfiTxpPTyi0n4YDRlLUA?0%5Babsolute%5D=1</a></div>
                  </td>

with info severity, markup:

        <td>
          Last run 1 hour 42 minutes ago
                      <div class="description">You can <a href="/drupal2/admin/reports/status/run-cron?token=8bQlufrbqSlG7jjBUwax2BKNXUI9PmLiV9iEG0zRbWk">run cron manually</a>.<br />To run cron from outside the site, go to <a href="/drupal2/cron/7Ws6Z_imhMrIUhb_lAcZq8UNA-1VRFIvMl8vbMbElrt5UCgcp-CRR2JfiTxpPTyi0n4YDRlLUA?0%5Babsolute%5D=1">/drupal2/cron/7Ws6Z_imhMrIUhb_lAcZq8UNA-1VRFIvMl8vbMbElrt5UCgcp-CRR2JfiTxpPTyi0n4YDRlLUA?0%5Babsolute%5D=1</a></div>
                  </td>

opened #2505499: Explicitly support render arrays in hook_requirements() for #59 4.

updated issue summary

added beta eval

and manual testing was done (including drush rq)

+++ b/core/modules/system/system.install
@@ -360,23 +358,29 @@ function system_requirements($phase) {
+          '@cron-handbook' => 'https://www.drupal.org/cron',
+          '@cron-manual' => \Drupal::url('system.run_cron'),
+          '@cron-outside' => \Drupal::url('system.cron', array('key' => \Drupal::state()->get('system.cron_key'), array('absolute' => TRUE))),

I like it how universal @cron placeholder has been separated into meaningful name placeholder names which makes it so much easier to read and understand.

Text is displayed correctly since cron ran pretty recently on my local.
I have read xjm's comment in #59 and all her concern has been addressed into the patch.

1. +++ b/core/modules/system/system.install
   @@ -360,23 +358,29 @@ function system_requirements($phase) {
   +      $description = t('Cron has not run recently. For more information, see the online handbook entry for <a href="@cron-handbook">configuring cron jobs</a>. You can <a href="@cron-manual">run cron manually</a>.<br />To run cron from outside the site, go to <a href="@cron-outside">@cron-outside</a>',
   ...
   +      $description = t('You can <a href="@cron-manual">run cron manually</a>.<br />To run cron from outside the site, go to <a href="@cron-outside">@cron-outside</a>',
   

   The reason this string was broken up into separate calls to t() was so that translators only had to translate each sentence once.

2. +++ b/core/modules/system/system.install
   @@ -360,23 +358,29 @@ function system_requirements($phase) {
   +        array(
   +          '@cron-handbook' => 'https://www.drupal.org/cron',
   +          '@cron-manual' => \Drupal::url('system.run_cron'),
   +          '@cron-outside' => \Drupal::url('system.cron', array('key' => \Drupal::state()->get('system.cron_key'), array('absolute' => TRUE))),
   +        )
   ...
   +        array(
   +          '@cron-manual' => \Drupal::url('system.run_cron'),
   +          '@cron-outside' => \Drupal::url('system.cron', array('key' => \Drupal::state()->get('system.cron_key'), array('absolute' => TRUE))),
   +        )
   

   If we're going to go down the t() route can we prepare the array of variables only once.

   That said the reason for ditching the inline twig approach was that:

   The inline template ... looks like too much.

   I'm still not convinced by that argument. I think the inline template approach kept the translation contexts the same and put the entire message and it's logic in one place. I think each of the cons of the inline template approach listed in the summary are refutable. Which I'd do in later comment because atm I'm time poor.

I think the patch in #56 was was fine and should be committed. We can't spend endless time bikeshedding the formatting code for 1 message.

It makes the minimal change needed to remove the SafeMarkup::set() call and minimizes the number of strings that get marked safe.

We have been trying to be thoughtful about translators, ... but for different reasons. Tagging to get someone to chime in.

@alexpott #65 2.

If we're going to go down the t() route can we prepare the array of variables only once.

The two arrays are different. I'm not sure how to prepare them just once.

--------------
I updated the issue summary to be more clear about which pros and cons surrounded translation.

Here is an excerpt

• Assemble the $description into an array, and use an inline template for rendering.
  • See an example implementation in comment #48
  • pros
    • For translators, No string duplication.
  • cons
    • For translators, Less context for translating each part of the paragraph.
  • For translators, no string changes.
• Use 2 giant strings to t
• See an example implementation in comment #61
• pros
  • Translation guidelines say it should be a big string: https://www.drupal.org/node/322774 (which basically implies that if a whole paragraph is translatable, it should all go inside the same t() call)
  • For translators: More context for the whole paragraph to better translate it, instead of sentence fragments.
• cons
  • For translators: some phrase duplication.
• For translators: string changes.
• use format() to concatenate strings
• See an example implementation in comment #39 and #56
• pros
  • For translators, No string duplication.
• cons
  • For translators, Less context for translating each part of the paragraph.
• For translators, no string changes.

At least 2 of the t() strings are re-used in core, so I think we should keep them separate as in #56, though in the second case make both @cron instead of !cron

used once: 'Cron has not run recently.'

core/modules/system/system.install
365:      $description = t('Cron has not run recently.') . ' ' . $help;

used 2x: 'You can <a href="@cron">run cron manually</a>.'

core/lib/Drupal/Core/Extension/module.api.php
774:    $requirements['cron']['description'] .= ' ' . t('You can <a href="@cron">run cron manually</a>.', array('@cron' => \Drupal::url('system.run_cron')));

core/modules/system/system.install
368:    $description .= ' ' . t('You can <a href="@cron">run cron manually</a>.', array('@cron' => \Drupal::url('system.run_cron')));

used 2x: 'To run cron from outside the site, go to <a href="!cron">!cron</a>'

core/modules/system/src/Form/CronForm.php
110:      '#markup' => '<p>' . t('To run cron from outside the site, go to <a href="!cron">!cron</a>', array('!cron' => $this->url('system.cron', array('key' => $this->state->get('system.cron_key')), array('absolute' => TRUE)))) . '</p>',

core/modules/system/system.install
369:    $description .= '<br />' . t('To run cron from outside the site, go to <a href="!cron">!cron</a>', array('!cron' => \Drupal::url('system.cron', array('key' => \Drupal::state()->get('system.cron_key'), array('absolute' => TRUE)))));

So I've no wish to hold up the removal of the SafeMarkup set for inline template in hook_requirements discussion. But also, I don't think was should be changing translation strings in this issue if we can help it - it is out of scope too.

I do think that we should try to simplify the logic so it's obvious what strings appear where and we only produce the variables for translatable strings once.

OK. But I could try to do a new patch with those clarifications.

That issue is different, though, because it's a case where one of the translated strings is returned from another function call (its value is not known). So that's really the only way to do it.

In this issue, it is known, so the options are:

$first_string = t('Some long string of text.');
$second_string = t('Some preamble. @some_text', ['@some_text' => $first_string]);

vs.

$first_string = t('Some long string of text.');
$second_string = t('Some preamble. Some long string of text.');

Where the first has the advantage that it avoids repetition, but the second has the advantage that it gives more context to the translation.

@all: Re string concatenation, I don't think either of the short strings are ambiguous. In #2501639: Remove SafeMarkup::set in drupal_check_module() the "Currently using !item !version" string was VERY confusing without context. This is not true for the strings here AFAIS. So I think either way works.

@YesCT: Re the array, there is only one item difference in the array (in 61 at least) and t() does not care for extra elements in the array, so you can build one array and reuse for t()s or build the common part of the two arrays and add the additional item only for the t() which needs it.

Should this be still postponed?

The issue seems to have been postponed on #1825952: Turn on twig autoescape by default and then my feedback. Both of which are now in.

that assign to @xjm was from June 13 ... and I'm guessing they dont still have patches to propose.

Reviewing this now.

No longer reviewing this; stefan_r will review.

In manual testing this seems to display the same as before.

Reviewing this now.

Updating proposed resolution part 2 in the summary to match what was discussed.

Fix link to comment in description.

Add manual testing steps to the summary.

1. Markup at /admin/reports/status w/no error

BEFORE:

<td>
Last run 2 minutes 11 seconds ago
<div class="description"> You can <a href="/admin/reports/status/run-cron?token=QmjA8Ul24gp8yia_2_ENIfCie1CNiyJjKRWk8o2ltWM">run cron manually</a>.<br>To run cron from outside the site, go to <a href="/cron/muPJfr9iGHe9My2ZOEAji0Yx7ylK8DpRvlVXHFaI5r9ifmQ42WClDtLMMBdPW_7_FN9EhTd6eA?0%5Babsolute%5D=1">/cron/muPJfr9iGHe9My2ZOEAji0Yx7ylK8DpRvlVXHFaI5r9ifmQ42WClDtLMMBdPW_7_FN9EhTd6eA?0%5Babsolute%5D=1</a></div>
</td>

AFTER:

<td>
Last run 2 minutes 55 seconds ago
<div class="description">You can <a href="/admin/reports/status/run-cron?token=QmjA8Ul24gp8yia_2_ENIfCie1CNiyJjKRWk8o2ltWM">run cron manually</a>.<br>To run cron from outside the site, go to <a href="/cron/muPJfr9iGHe9My2ZOEAji0Yx7ylK8DpRvlVXHFaI5r9ifmQ42WClDtLMMBdPW_7_FN9EhTd6eA?0%5Babsolute%5D=1">/cron/muPJfr9iGHe9My2ZOEAji0Yx7ylK8DpRvlVXHFaI5r9ifmQ42WClDtLMMBdPW_7_FN9EhTd6eA?0%5Babsolute%5D=1</a></div>
</td>

2. drush rq w/no error

BEFORE:

 Cron maintenance tasks  Info      Last run 10 minutes 12 seconds ago
                                    You can run cron manually.To run cron from outside the site, go to
                                   /cron/muPJfr9iGHe9My2ZOEAji0Yx7ylK8DpRvlVXHFaI5r9ifmQ42WClDtLMMBdPW_7_FN9EhTd6eA?0%5Babsolute%5D=1

AFTER:

 Cron maintenance tasks   Info      Last run 8 minutes 44 seconds ago
                                    Array

3. Markup at /admin/reports/status with error

BEFORE:

<td>
Last run 19 minutes 3 seconds ago
<div class="description">Cron has not run recently. For more information, see the online handbook entry for <a href="https://www.drupal.org/cron">configuring cron jobs</a>. You can <a href="/admin/reports/status/run-cron?token=QmjA8Ul24gp8yia_2_ENIfCie1CNiyJjKRWk8o2ltWM">run cron manually</a>.<br>To run cron from outside the site, go to <a href="/cron/muPJfr9iGHe9My2ZOEAji0Yx7ylK8DpRvlVXHFaI5r9ifmQ42WClDtLMMBdPW_7_FN9EhTd6eA?0%5Babsolute%5D=1">/cron/muPJfr9iGHe9My2ZOEAji0Yx7ylK8DpRvlVXHFaI5r9ifmQ42WClDtLMMBdPW_7_FN9EhTd6eA?0%5Babsolute%5D=1</a></div>
</td>

AFTER:

<td>
Last run 21 minutes 46 seconds ago
<div class="description">Cron has not run recently. For more information, see the online handbook entry for <a href="@cron-handbook">configuring cron jobs</a>. You can <a href="/admin/reports/status/run-cron?token=QmjA8Ul24gp8yia_2_ENIfCie1CNiyJjKRWk8o2ltWM">run cron manually</a>.<br>To run cron from outside the site, go to <a href="/cron/muPJfr9iGHe9My2ZOEAji0Yx7ylK8DpRvlVXHFaI5r9ifmQ42WClDtLMMBdPW_7_FN9EhTd6eA?0%5Babsolute%5D=1">/cron/muPJfr9iGHe9My2ZOEAji0Yx7ylK8DpRvlVXHFaI5r9ifmQ42WClDtLMMBdPW_7_FN9EhTd6eA?0%5Babsolute%5D=1</a></div>
</td>

4. drush rq with error

BEFORE:

 Cron maintenance tasks  Error     Last run 18 minutes 2 seconds ago
                                   Cron has not run recently. For more information, see the online handbook entry for configuring cron jobs. You can run cron manually.To run cron
                                   from outside the site, go to /cron/muPJfr9iGHe9My2ZOEAji0Yx7ylK8DpRvlVXHFaI5r9ifmQ42WClDtLMMBdPW_7_FN9EhTd6eA?0%5Babsolute%5D=1

AFTER:

 Cron maintenance tasks   Error     Last run 20 minutes 23 seconds ago
                                    Array

From my manual testing results in #96, I see these issues:

1. drush rq displays an array instead of the text
2. the @cron-handbook link is not getting replaced

Setting to needs work.

Is the drush problem in scope here?

I just checked with @alexpott on IRC and the drush issue looks to be out of scope here, let's file a PR at https://github.com/drush-ops/drush

Reviewing and manually testing now.

Here are my manual testing results for patch #98. Note that the "BEFORE" is not mentioned here since it should be the same as the previous results, see #96.

1. Markup at /admin/reports/status w/no error

AFTER:

<td>
Last run 55 minutes 45 seconds ago
<div class="description">You can <a href="/admin/reports/status/run-cron?token=QmjA8Ul24gp8yia_2_ENIfCie1CNiyJjKRWk8o2ltWM">run cron manually</a>.<br>To run cron from outside the site, go to <a href="/cron/muPJfr9iGHe9My2ZOEAji0Yx7ylK8DpRvlVXHFaI5r9ifmQ42WClDtLMMBdPW_7_FN9EhTd6eA?0%5Babsolute%5D=1">/cron/muPJfr9iGHe9My2ZOEAji0Yx7ylK8DpRvlVXHFaI5r9ifmQ42WClDtLMMBdPW_7_FN9EhTd6eA?0%5Babsolute%5D=1</a></div>
</td>

2. drush rq w/no error

AFTER:

 Cron maintenance tasks   Info      Last run 54 minutes 53 seconds ago
                                    Array

3. Markup at /admin/reports/status with error

AFTER:

<td>
Last run 58 minutes 37 seconds ago
<div class="description">Cron has not run recently. For more information, see the online handbook entry for <a href="https://www.drupal.org/cron">configuring cron jobs</a>. You can <a href="/admin/reports/status/run-cron?token=QmjA8Ul24gp8yia_2_ENIfCie1CNiyJjKRWk8o2ltWM">run cron manually</a>.<br>To run cron from outside the site, go to <a href="/cron/muPJfr9iGHe9My2ZOEAji0Yx7ylK8DpRvlVXHFaI5r9ifmQ42WClDtLMMBdPW_7_FN9EhTd6eA?0%5Babsolute%5D=1">/cron/muPJfr9iGHe9My2ZOEAji0Yx7ylK8DpRvlVXHFaI5r9ifmQ42WClDtLMMBdPW_7_FN9EhTd6eA?0%5Babsolute%5D=1</a></div>
</td>

4. drush rq with error

AFTER:

 Cron maintenance tasks   Error     Last run 57 minutes 58 seconds ago
                                    Array

Updating issue summary to reflex the resolution we are going with.

Marking remaining task as done.

I manually tested #98 and saw that it fixed issue #2 mentioned in #97.

Because issue #1 from #97 is being addressed by drush, see the pull request and issue, I believe all issues are covered.

Setting to RTBC.

I was confused by the summary. It only mentions the use of SafeMarkup used when setting the $description, however, there is a second use when printing phpinfo. See these lines:

 140     if ($phase === 'runtime') {
 141       $phpversion_label = SafeMarkup::set($phpversion . ' (' . \Drupal::l(t('more information'), new Url('system.php')) . ')');
 142     }

This needs removed as well since there is no other issue that addresses it.

Setting back to needs work.

Updating summary to clarify there are more than one calls to SafeMarkup.

Thanks @josephdpurcell!

1. +++ b/core/modules/system/system.install
   @@ -134,28 +134,27 @@ function system_requirements($phase) {
   +  $phpversion = phpversion();
   

   $phpversion is used only once? If so, not necessary to assign to a variable.

2. +++ b/core/modules/system/system.install
   @@ -134,28 +134,27 @@ function system_requirements($phase) {
   +      '#markup' => t('For more information, view the <a href="@php-info">phpinfo</a>', ['@php-info', '/admin/reports/status/php']),
   

   The wording here has changed and has been moved from the value to the description. Can we post a screenshot with the visual change?

   Also misses a full stop.

3. +++ b/core/modules/system/system.install
   @@ -134,28 +134,27 @@ function system_requirements($phase) {
   
   +      '#markup' => t('Your PHP installation is too old. Drupal requires at least PHP %version.', ['%version' => DRUPAL_MINIMUM_PHP]),
   
   @@ -164,12 +163,11 @@ function system_requirements($phase) {
   +      '#markup' => t('PHP versions higher than 5.6.5 or 5.5.21 provide built-in SQL injection protection for mysql databases. It is recommended to update.'),
   

   Just so I can understand, as they don't have any links, why are these necessary?

4. +++ b/core/modules/system/system.install
   @@ -134,28 +134,27 @@ function system_requirements($phase) {
   +      '#markup' => t('For more information, view the <a href="@php-info">phpinfo</a>', ['@php-info', '/admin/reports/status/php']),
   

   You hardcoded the link here, this is likely making tests fail. This will also break on sites that run on a prefixed URL.

5. This could have been a 3 line patch if we merely changed $requirement['php']['value'] to be a markup array... moving the phpinfo link to the description looks fine as well, but maybe we can offer some explanation as to why we are changing this?
6. You're doing $requirement['php']['description'][], so this will append the description to the previously defined description instead of overwriting it.
7. Unused SafeMarkup use statement

1. $phpversion is used at least twice, sometimes three.
2. See screenshots below.
3. In short, I don't know. Would these indicators show up anywhere else?
4. Good catch. Fixed.
5. The intent of the 'value' field is a version, time, level, etc (see docs). It seems that having markup in 'value' would be against the purpose of this field. Instead, putting it in the description where markup is allowed seems like a good fit. If 'value' is fine to have markup in, then yes, we could do away with these changes and just throw markup in there.
6. Yes, the reason for doing this is because previously the phpinfo link was always available. To continue making that available, I'm appending. However, this appending seems to clutter the messages as seen in the screenshots. I'm wondering if it would make sense to change this up to make it cleaner, I'll present a follow up patch to suggest what that might look like.
7. Ah! That was the point wasn't it. Fixed.

Screenshots

Whoops, I left in the manual testing changes to make the screenshots.

This patch puts back the real code.

Yes, better to stick to the scope of the issue so as to not drag this out too much, we can then improve the message in a followup :)

I think putting a markup array in the value key is perfectly fine, we put markup in there now as well (as a string). The docs are a bit confusing -- I would read them as giving examples of what you /can/ put in there, rather than what you /must only/ put in there. So if anything the docs could use updating...

Also, I think we forgot a "before" screenshot?

Ok, here is what it would look like if we just did a render array.

And the screenshots:

BEFORE:

AFTER:

Note: I didn't do all 6 cases since the code only changed for this one case.

Great :) For me this can go back to RTBC if green...

This looks good, least amount of changes to get it done. Nice working removing SafeMarkup as a dependency on that that file!

This is a really nice fix. The translated strings have not changed and the neither has the logic. Since this only effects runtime checks I think we don't have to wait for #2505499: Explicitly support render arrays in hook_requirements(). Two less SafeMarkup::set() calls. Nice. Committed 6f106ac and pushed to 8.0.x. Thanks!

So @xjm pointed out

+++ b/core/modules/system/system.install
@@ -138,7 +137,9 @@ function system_requirements($phase) {
-      $phpversion_label = SafeMarkup::set($phpversion . ' (' . \Drupal::l(t('more information'), new Url('system.php')) . ')');
+      $phpversion_label = [
+        '#markup' => $phpversion . ' (' . \Drupal::l(t('more information'), new Url('system.php')) . ')',
+      ];

This is a value not a description. In fact let's file a separate issue to deal with this an not include it in the patch.

This patch removes the link entirely. I have created a follow up task to put the link back in #2551725: Remove system_requirements() SafeMarkup::set() use with 'value' key.

Ignore that last patch, I misunderstood what we were doing.

Okay, so this patch takes us all the way back to #98, i.e. this patch is identical to #98.

The second SafeMarkup::set that #119 meant to address will be worked on in #2551725: Remove system_requirements() SafeMarkup::set() use with 'value' key.

Back to RTBC, and back to #98

just wondering what's wrong with it being a value? The value field should still be able to have markup (it did for PHP before the patch), so if anything don't the docs need updating to reflect render arrays are fine there? We were working on test coverage for this in the child issue.

Ah nevermind, just noticed we have #2549803: hook_requirements() 'value' key is confusing and used inconsistently, and 'title' is only displayed during installation if the value is also set

+++ b/core/modules/system/system.install
@@ -440,17 +440,22 @@
+      '#weight' => 2
...
+      '#weight' => 3

Nit: Need some ending commas on these array items.

I knew it!

+    if ($severity != REQUIREMENT_INFO) {
+      $requirements['cron']['description'][] = ['#markup' => t('Cron has not run recently.'), '#weight' => 0];
+      $requirements['cron']['description'][] = [
+        '#prefix' => ' ',
+        '#markup' => t('For more information, see the online handbook entry for <a href="@cron-handbook">configuring cron jobs</a>.', ['@cron-handbook' => 'https://www.drupal.org/cron']),
+        '#suffix' => ' ',
+        '#weight' => 1,
+      ];
+    }
+    $requirements['cron']['description'][] = [
+      '#markup' => t('You can <a href="@cron">run cron manually</a>.', ['@cron' => \Drupal::url('system.run_cron')]),
+      '#weight' => 2,
+    ];

I do not understand how we've come to a point where the above is required when you simply want to write a paragraph with 3 dynamic sentences in it???!!!

If it's going to be a render array, couldn't the strings at least be concatenated in PHP (like everyone else does everywhere) and then simply this:

  $requirements['cron']['description']['#markup'] = $the_concatenated_string;

Yes, it means that somewhere down the line, the render API would do a xssFilterAdmin on this when technically it isn't needed, but is that really a big deal compared to the much simpler developer experience? Concatenating strings like this isn't too common anyway, so the extra xssFilterAdmin call wouldn't happen too often.

Nevertheless, I see this pattern is used in core at least once already, so if this needs to be a followup issue instead, then so be it...

@David_Rothstein because the t() returns SafeMarkup and it has <a> tag that once concatenated will lose its "Safeness" and with Xss::filterAdmin() via #markup would be filtered out.

Also, you aren't crazy it's horrible DX.

@joelpittet, Xss::filterAdmin() won't filter out <a> tags; it allows pretty much everything to pass through except some dangerous/invalid/uncommon tags. It would only not work in the rare situation that you need to use one of those disallowed tags.

In code terms my suggestion is that this whole patch could just become:

diff --git a/core/modules/system/system.install b/core/modules/system/system.install
index f38af8a..1785bb1 100644
--- a/core/modules/system/system.install
+++ b/core/modules/system/system.install
@@ -447,11 +447,7 @@ function system_requirements($phase) {
       'title' => t('Cron maintenance tasks'),
       'severity' => $severity,
       'value' => $summary,
-      // @todo This string is concatenated from t() calls, safe drupal_render()
-      //   output, whitespace, and <br /> tags, so is safe. However, as a best
-      //   practice, we should not use SafeMarkup::set() around a variable. Fix
-      //   in: https://www.drupal.org/node/2296929.
-      'description' => SafeMarkup::set($description),
+      'description' => array('#markup' => $description),
     );
   }
   if ($phase != 'install') {

I tested it now and it seems to work fine.

Either way, glad I'm not crazy :)

@David_Rothstein oh my bad I'm wrong about links filtered out, just their possibly nasty href="javascript:" and onclick type attributes are stripped., glad I looked at it again.

Also, I like you're idea @David_Rothstein in #140, @akalata and @xjm want to give that try and see if we can get that potentially better DX working?

Hoping everybody agrees this is a bit more sane... I know @akalata said she was busy for a couple of days on client work so I'm rolling a patch so others can have a review.

I converted the ! to @ too because they ensure the unsafe values in those variables are escaped.

I don't really understand what would be wrong with a single markup array here, it looks fine to me! We did have this same discussion last thursday while discussing this issue:

11:56 PM <stefan_r> wait, why dont we just make this a 1 line fix?
11:57 PM <stefan_r> -      'description' => SafeMarkup::set($description),
11:57 PM <stefan_r> +      'description' => ['#markup' => $description],
11:57 PM <xjm> stefan_r:  because then that is escaping and then filtering again
11:57 PM <xjm> which is unneccessary
11:57 PM <xjm> stefan_r: if we have a #markup for each t() we avoid that and also it's more readable
11:58 PM <stefan_r> so an array of #markups
11:58 PM <xjm> yep!
11:58 PM <xjm> stefan_r: and I think one #prefix

+++ b/core/modules/system/system.install
@@ -434,24 +434,22 @@ function system_requirements($phase) {
+    $description .= '<br />' . t('To run cron from outside the site, go to <a href="@cron">!cron</a>', array('@cron' => \Drupal::url('system.cron', array('key' => \Drupal::state()->get('system.cron_key'), array('absolute' => TRUE)))));

This where we should be doing url filtering. Because this does not really do anything apart from an call htmlspecialchars() that does not do much. Not sure what's the right thing to do here.

Also we still have !cron in the string.

Done a bit more thinking about !cron issue. It's not in scope for this issue - it should be part of #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand. The output of \Drupal::url('system.cron', array('key' => \Drupal::state()->get('system.cron_key'), array('absolute' => TRUE)) can not be unsafe.

voila

The latest patch looks nice and simple and accomplishes the goals. By making the description a render array it is going to admin filtered - which seems fine since this is definitely admin content.

Okay I prefer #136. I missed the previous discussion and did not think hard enough. My bad. #136 does not need the #weight though.

I've been thinking about @David_Rothstein's objections to the complexity of multiple render arrays with #markup. How about we allow #markup to be a array of strings that we join safely. Here's an issue that shows what I mean - #2554073: Allow #markup to be an array of strings and join them safely

I agree with #156. If #2554073: Allow #markup to be an array of strings and join them safely lands then we can simplify all of this post 8.0.x for all it is worth. And yes it ugly but it is the way we've solved the problem before. Sorry again for my earlier rtbc in #150.

We are working on #2505499: Explicitly support render arrays in hook_requirements(). Render arrays are our current solution for this. The patch keeps all the translatable strings the same. Let's do it.

it has minimal side effects.

except for projectile vomiting and a boggling DX WTF, yes I agree.

#143 still looks like the best choice. I honestly don't see what we are gaining from moving these strings to render arrays.

@xjm can you explain #147

OK... but we're trading one problematic pattern for another. =/

Especially the part where the first pattern is problematic.

I think this needs a bit more discussion, because I really don't see the problem with the approach in #143 and others have thought that was a good solution, though rightfully questioning it as a solution, it seems to be an elegant solution. I'm sure @David_Rothstein is on board with that version and both @xjm and @alexpott had thought that was ok as well... so I'm really curious what was the clincher.

FTR: Still hate this render array approach, but between render arrays and SafeMarkup::set() removal, the latter wins, and at least we know our hands are not tied with that approach in contrib/IRL.

Follow up issue #2563757: External cron URL on status page is not absolute.