Follow-up to #1825952: Turn on twig autoescape by default

Problem/Motivation

In system_requirements(), we mark as safe a long $description variable containing concatenated t() calls, whitespace, <br /> tags, drupal_render()calls, etc. We also mark the php version text as safe. As a best practice, , we should not use SafeMarkup::set() around a variable.

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 every SafeMarkup::set() call
Unfrozen changes Unfrozen because it only changes strings and tests

Proposed resolution

Many resolutions were discussed. The path decided on is to use a render array, leave the uses of t(), and remove the SafeMarkup::set call.

Follow-up for other approach: #2505499: Explicitly support render arrays in hook_requirements()

Remaining tasks

Update summary explaining various proposed resolutions.

User interface changes

None.

admin/reports/status

Manual testing steps

  1. Confirm no change to the markup at /admin/reports/status "Cron maintenance tasks" section
  2. Confirm no change to the CLI output by running "drush rq" and looking at the "Cron maintenance tasks" section
  3. Repeat the above two tasks after changing core/modules/system/system.install to think there has been an error by adding this line: 433 $severity = REQUIREMENT_ERROR;

head

API changes

None.

Files: 
CommentFileSizeAuthor
#154 interdiff-136-153.txt1.97 KBstefan.r
#153 2296929-136-without-weights-153.patch3 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,199 pass(es). View
#152 2296929-136-without-weights-152.patch2.98 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,197 pass(es). View
#151 2296929-136-without-weights.patch2.96 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,177 pass(es). View
#148 2296929-148.patch957 bytesstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,208 pass(es). View
#143 remove-2296929-143.patch2.03 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,182 pass(es). View
#136 interdiff-134-136.txt789 bytesakalata
#136 2296929-136.patch3.09 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,230 pass(es). View
#134 interdiff-129-134.txt1.48 KBakalata
#134 2296929-134.patch3.08 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,232 pass(es). View
#129 interdiff-2296929-128-129.txt978 bytesjosephdpurcell
#129 2296929-129.patch2.99 KBjosephdpurcell
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,204 pass(es). View
#128 interdiff-2296929-119-128.txt1.94 KBjosephdpurcell
#128 2296929-128.patch4.95 KBjosephdpurcell
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,209 pass(es), 1 fail(s), and 0 exception(s). View
#120 Screen Shot 2015-08-14 at 12.26.19 PM.png18.82 KBjosephdpurcell
#120 Screen Shot 2015-08-14 at 12.25.34 PM.png18.88 KBjosephdpurcell
#119 interdiff-2296929-98-119.txt978 bytesjosephdpurcell
#119 2296929-119.patch3.75 KBjosephdpurcell
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,185 pass(es). View
#116 interdiff-2296929-115-116.txt1.03 KBjosephdpurcell
#116 2296929-116.patch66.68 KBjosephdpurcell
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 35,346 pass(es), 1,021 fail(s), and 796 exception(s). View
#115 interdiff-2296929-110-115.txt2.68 KBjosephdpurcell
#115 2296929-115.patch66.71 KBjosephdpurcell
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#115 Screen Shot 2015-08-14 at 11.35.40 AM.png36.03 KBjosephdpurcell
#115 Screen Shot 2015-08-14 at 11.36.29 AM.png68.4 KBjosephdpurcell
#115 Screen Shot 2015-08-14 at 11.36.16 AM.png59.59 KBjosephdpurcell
#115 Screen Shot 2015-08-14 at 11.36.05 AM.png47.67 KBjosephdpurcell
#115 Screen Shot 2015-08-14 at 11.35.52 AM.png44.04 KBjosephdpurcell
#115 Screen Shot 2015-08-14 at 11.35.23 AM.png24.06 KBjosephdpurcell
#111 interdiff-2296929-98-110.txt3.6 KBjosephdpurcell
#110 2296929-110.patch6.39 KBjosephdpurcell
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,534 pass(es), 1 fail(s), and 0 exception(s). View
#98 interdiff-87-98.txt726 bytesstefan.r
#98 2296929-98.patch2.99 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,539 pass(es). View
#89 2296929-87.patch2.94 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,647 pass(es). View
#89 interdiff-86-87.txt517 bytesstefan.r
#88 interdiff-61-86.patch5.67 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch interdiff-61-86.patch. Unable to apply patch. See the log in the details link for more information. View
#88 2296929-86.patch2.94 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,641 pass(es). View
#64 Screen Shot 2015-06-13 at 1.03.57 AM.png54.72 KBkgoel
#61 interdiff.2296929.56.61.txt3.31 KBYesCT
#61 2296929.61.patch3.1 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,838 pass(es). View
#56 interdiff-2296929-52-54.txt1.68 KBYesCT
#56 clean_up-2296929-54.patch2.65 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,846 pass(es). View
#53 clean_up-2296929-52.patch2.52 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,844 pass(es). View
#52 patch-info.png63.11 KBYesCT
#52 head-info.png57.35 KBYesCT
#52 format-patch.png306.65 KBYesCT
#52 t-head.png299.14 KBYesCT
#50 no_break_no_warning.png6.25 KBcodemonkie
#50 with_break_no_warning.png6.69 KBcodemonkie
#48 interdiff-2296929-34-48.txt1.77 KBcodemonkie
#48 clean_up-2296929-48.patch3.29 KBcodemonkie
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,263 pass(es). View
#39 2296929-39.patch2.92 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,500 pass(es). View
#34 2296929.34.patch3.07 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,286 pass(es). View
#34 33-34-interdiff.txt1.09 KBalexpott
#33 2296929.33.patch3.07 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,276 pass(es). View
#33 32-33-interdiff.txt3.07 KBalexpott
implode_system_install_description.patch10.44 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch implode_system_install_description.patch. Unable to apply patch. See the log in the details link for more information. View
#9 cleanup_system_install_safe_markup-2296929-08.patch1.13 KBaneek
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,766 pass(es). View
#12 clean_up-2296929-12.patch1.13 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,883 pass(es). View
#17 clean_up-2296929-17.patch2.07 KBherved
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,581 pass(es). View
#22 interdiff-2296929-12-17.txt2.12 KBherved
#26 clean_up-2296929-26.patch2.36 KBcodemonkie
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch clean_up-2296929-26.patch. Unable to apply patch. See the log in the details link for more information. View
#26 interdiff-2296929-17-26.txt1.25 KBcodemonkie
#30 clean_up-2296929-30.patch2.36 KBcodemonkie
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,113 pass(es). View
#32 interdiff-2296929-30-32.txt2.44 KBcodemonkie
#32 clean_up-2296929-32.patch2.55 KBcodemonkie
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,291 pass(es). View

Comments

xjm’s picture

Title: Turn on twig autoescape by default. » Clean up system_install() SafeMarkup::set() use

Issue clone fail.

xjm’s picture

Upon reflection, that patch is probably totally the wrong way to do this. We should look into why this got added here in the first place; maybe double-escaping some <br /> tags that we maybe shouldn't be adding anyway?

xjm’s picture

Status: Postponed » Active
jibran’s picture

Issue tags: +SafeMarkup
arpitr’s picture

Assigned: Unassigned » arpitr
Issue tags: +drupalgoa2015
arpitr’s picture

Assigned: arpitr » Unassigned
Status: Active » Needs review

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

aneek’s picture

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

Status: Needs review » Needs work

The last submitted patch, implode_system_install_description.patch, failed testing.

aneek’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,766 pass(es). View

Uploading a new patch. Review!!

arpitr’s picture

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

Cottser’s picture

Status: Needs review » Needs work

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)

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,883 pass(es). View

Fixed the standards

aneek’s picture

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

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs 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.

aneek’s picture

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

herved’s picture

FileSize
2.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,581 pass(es). View

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.

Status: Needs review » Needs work

The last submitted patch, 17: clean_up-2296929-17.patch, failed testing.

Status: Needs work » Needs review

herved queued 17: clean_up-2296929-17.patch for re-testing.

herved’s picture

Not sure what happened here with the testbot O_o

aneek’s picture

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

herved’s picture

FileSize
2.12 KB

Sure here it is.

pfrenssen’s picture

Status: Needs review » Needs work
  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.

pfrenssen’s picture

Mile23’s picture

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.

codemonkie’s picture

Issue tags: +LosAngeles2015
FileSize
2.36 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch clean_up-2296929-26.patch. Unable to apply patch. See the log in the details link for more information. View
1.25 KB

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.

xjm’s picture

xjm’s picture

Priority: Normal » Major

Status: Needs review » Needs work

The last submitted patch, 26: clean_up-2296929-26.patch, failed testing.

codemonkie’s picture

FileSize
2.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,113 pass(es). View

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)

Cottser’s picture

Status: Needs work » Needs review

Setting to needs review so it gets tested. Thanks!

codemonkie’s picture

Issue summary: View changes
FileSize
2.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,291 pass(es). View
2.44 KB

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)

alexpott’s picture

FileSize
3.07 KB
3.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,276 pass(es). View

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

alexpott’s picture

FileSize
1.09 KB
3.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,286 pass(es). View

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.

xjm’s picture

Issue tags: +Needs manual testing

I like the approach in #34 more and more, but it's definitely different from how we've composed HTML for messages previously. People are for sure used to seeing t(). It could be good to make it so people get used to seeing Twig, and it's certainly an advantage to have the markup, conditions, and translations all in one place. It definitely reduces the risk of composing HTML out of strings containing variables, and also reduces the chance that we'll end up with nested string format/translation calls.

We need to keep in mind a few things if we adopt this pattern. One is translations:

+++ b/core/modules/system/system.install
@@ -351,23 +349,31 @@ function system_requirements($phase) {
-      $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')));
...
+{% if run_warning %}{% trans %}Cron has not run recently. For more information, see the online handbook entry for <a href="{{ cron_handbook|passthrough }}">configuring cron jobs</a>.{% endtrans %} {% endif %}{% trans %}You can <a href="{{ cron_inside|passthrough }}">run cron manually</a>.{% endtrans %}<br />

This is changing the strings for translators. Previously, there were two meaningful strings: "Cron has not run recently" and "For more information,..." were separate strings for translators. Now we are merging them into one new string which will need a different translation. The second string is also used elsewhere in core, which means translators will have to re-translate almost exactly the same text. So it might be better to retain this as two separate translated strings, even though they are right next to each other.

Another consideration is readability.

+++ b/core/modules/system/system.install
@@ -351,23 +349,31 @@ function system_requirements($phase) {
+{% if run_warning %}{% trans %}Cron has not run recently. For more information, see the online handbook entry for <a href="{{ cron_handbook|passthrough }}">configuring cron jobs</a>.{% endtrans %} {% endif %}{% trans %}You can <a href="{{ cron_inside|passthrough }}">run cron manually</a>.{% endtrans %}<br />

The Twig template is all on one long line that is difficult to scan/parse. Would it be appropariate or feasible to break it up onto multiple lines, e.g. so the if and endif were on their own lines? Does that cause problems with whitespace?

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

A final, broader consideration is documentation. Is the documentation for inline templates available when you're reading the documentation for t() and SafeMarkup::format()? This is out of scope for this issue, but if we're going to use inline templates as more than a temporary hack before one moves actual markup into a "proper" template, we should double-check that we have documentation in the codebase that explains the difference between these two methods of composing strings.

I'd suggest testing this manually on /admin/reports/status and with drush. I also would double-check how the strings are available to translators. (The Twig trans tag is hopefully scanned exactly like t() is? But would be good to double-check if we're doing the translation inside an inline template.)

If this works, and @Cottser is okay with it (maybe double-check with @Gábor Hojtsy or another multilingual contributor on the translation bit too), maybe this is a pattern we could use elsewhere for #2280965: [meta] Remove every SafeMarkup::set() call as well.

Cottser’s picture

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 :)

xjm’s picture

effulgentsia’s picture

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.

David_Rothstein’s picture

FileSize
2.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,500 pass(es). View

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.

alexpott’s picture

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.

Cottser’s picture

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.

effulgentsia’s picture

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?

David_Rothstein’s picture

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 . '
' . 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?

David_Rothstein’s picture

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.

effulgentsia’s picture

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.

xjm’s picture

Thanks @David_Rothstein and @effulgentsia.

Radical notion, but why is the <br /> tag there at all? A before-and-after screenshot of whether it impacts the usability might be informative.

+++ 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));

Yeach. I really don't think we should do that. It sets an example that is, like, worst practice.

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. 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. So my vote would be to update the documentation for the hook as @effulgentsia suggests (in a separate issue).

I read @effulgentsia's comment on #2280965-20: [meta] Remove every SafeMarkup::set() call and @dawehner's response in #21 and will follow up over there, but what I think we should avoid is nested calls that add separate strings to the safe list and then their composite on top of that.

effulgentsia’s picture

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']?

codemonkie’s picture

FileSize
3.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,263 pass(es). View
1.77 KB

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.

David_Rothstein’s picture

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.

codemonkie’s picture

@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.

YesCT’s picture

Status: Needs review » Needs work

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.

YesCT’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
299.14 KB
306.65 KB
57.35 KB
63.11 KB

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

        
          Last run 1 hour 22 minutes 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 /drupal/cron/6_P1AmCzM4u9__Jo-i5K3rhckWBOxCliZ4HQCIlUiRyAsSliaUwtegqYk7OiNlE6Ubj0A1z9aw?0%5Babsolute%5D=1

patch

markup

        
          Last run 1 hour 23 minutes 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 /drupal2/cron/Te4FsQTQJHggRz18AXwWh7Zu8v5GSjDYeJ3tt8Unj1PIr2-OeNGb3xHGhTVno8mzeDSYFbCMtw?0%5Babsolute%5D=1

severity: info

head

markup

        
          Last run 1 hour 13 minutes ago
                      
                  

patch

markup

        
          Last run 1 hour 15 minutes ago
                      
                  

=====
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 run cron manually.', 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]

YesCT’s picture

FileSize
2.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,844 pass(es). View

Now with patch.

kgoel’s picture

Status: Needs review » Needs work
  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.

kgoel’s picture

YesCT’s picture

Status: Needs work » Needs review
FileSize
2.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,846 pass(es). View
1.68 KB

address both of those points.

David_Rothstein’s picture

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

@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.

xjm’s picture

Status: Needs review » Needs work

So, I'm still uncomfortable with this pattern (of translated strings globbed together around whitespace inside a SafeMarkup::set()) and I still think it would be worth considering:

  1. @alexpott's solution with the inline template
  2. Just making this one t() call
  3. Removing the <br /> tag.
  4. Evaluating whether we should explicitly support render arrays in hook_requirements() and the like.

I'm leaning toward #2 (after checking with a translator, as suggested) and maybe #3, with more general followups for #1 and #4.

That aside, looking at the specific patch, I think we should always avoid this in particular:

+++ b/core/modules/system/system.install
@@ -360,23 +360,33 @@ function system_requirements($phase) {
+      $description_string = '@description_warning @help ';
...
+    $description_replacements += [
+      '@description_manual' => $description_manual,
+      '@description_additional_info' => $description_additional_info,
+    ];
+    $description_string .= '@description_manual<br />@description_additional_info';
+    $description = SafeMarkup::format($description_string, $description_replacements);

$description_string and $description_replacements should not be defined as separate variables. Even if we do use this pattern of SafeMarkup::format(@thing_1 @thing_2) (which I don't think we should), the top-level arguments of SafeMarkup::format() should not be variables. It makes the code more difficult to read, more difficult to security audit, and more likely to promote bad practices with string sanitization. We should continue to follow the same rule we use for t() and format_string() in D7 that the first argument should always be a string literal and the replacement tokens should be clear as well.

The less we compose variables across many lines, the better. It'd better to have multiple SafeMarkup::format() or t() calls in different code paths if necessary, rather than composing strings across many lines of code.

Thanks everyone for your patience here! It's definitely not a straightforward thing.

YesCT’s picture

ok. I'll try doing 2. now.

YesCT’s picture

Status: Needs work » Needs review
FileSize
3.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,838 pass(es). View
3.31 KB

ok, this does 2.

with warning severity, markup:

          Last run 1 hour 38 minutes 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 /drupal2/cron/7Ws6Z_imhMrIUhb_lAcZq8UNA-1VRFIvMl8vbMbElrt5UCgcp-CRR2JfiTxpPTyi0n4YDRlLUA?0%5Babsolute%5D=1

with info severity, markup:

        
          Last run 1 hour 42 minutes ago
                      
                  
YesCT’s picture

YesCT’s picture

updated issue summary

added beta eval

and manual testing was done (including drush rq)

kgoel’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
54.72 KB
+++ 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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

pwolanin’s picture

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.

YesCT’s picture

Issue tags: +D8MI

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

YesCT’s picture

Issue summary: View changes

@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.
pwolanin’s picture

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)))));
xjm’s picture

So what @alexpott is asking for in #65 point 2 is to store these to variables once:

\Drupal::url('system.cron', array('key' => \Drupal::state()->get('system.cron_key'), array('absolute' => TRUE))),
\Drupal::url('system.run_cron'),
xjm’s picture

I also like the idea of using an inline template, as I've said many times on this issue. However, it looks like using that approach doesn't have consensus from other stakeholders (@David_Rothstein, @effulgentsia, the Twig maintainers), which is why I suggested going the simpler route. I tihnk we should discuss inline templates or templates for "simple" string logic in a separate issue for that reason, and not block this patch on it.

@alexpott, regarding #65 point 1, that is exactly what we have been discussing on this issue, and why one of the remaining tasks was to get input from a translator. I think it should be at NR until that happens.

Regarding #69, here is what I'd do:

  1. +++ b/core/modules/system/system.install
    @@ -360,23 +358,29 @@ function system_requirements($phase) {
    +      $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>',
    

    Do this once before the if statement and store it to $description, because it is one logical translatable string.

  2. +++ 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>',
    

    Change this to t('Cron has not run recently. ....@run_cron'), where @run_cron is the $description above.

xjm’s picture

Issue summary: View changes

Just making the summary list an <ol> so we can refer to specific points.

alexpott’s picture

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.

xjm’s picture

Assigned: Unassigned » xjm
Status: Needs work » Postponed

Assigning this to myself so I can propose some patches. @alexpott and I discussed this for awhile.

YesCT’s picture

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

xjm’s picture

I've pinged @Gábor Hojtsy in #2501639: Remove SafeMarkup::set in drupal_check_module() for feedback on the use there of

t('something something something. @another_thing', ['@another_thing' => $some_other_previously_translated_thing])

That will inform the most correct fix here as well (i.e. whether #71 is part of a correct solution).

davidhernandez’s picture

Issue tags: +D8 Accelerate
David_Rothstein’s picture

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.

Gábor Hojtsy’s picture

@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.

lauriii’s picture

Should this be still postponed?

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Postponed » Needs work

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.

YesCT’s picture

Assigned: xjm » Unassigned

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

xjm’s picture

Assigned: Unassigned » xjm

Picking this back up; sorry for ignoring it for a month.

xjm’s picture

Title: Clean up system_install() SafeMarkup::set() use » Remove system_requirements() SafeMarkup::set() use

Also the title for this keeps goofing me up.

xjm’s picture

Assigned: xjm » Unassigned

So now that #2505499: Explicitly support render arrays in hook_requirements() is going forward, we can update this to be a render array too. :)

josephdpurcell’s picture

Reviewing this now.

josephdpurcell’s picture

No longer reviewing this; stefan_r will review.

stefan.r’s picture

FileSize
2.94 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,641 pass(es). View
5.67 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch interdiff-61-86.patch. Unable to apply patch. See the log in the details link for more information. View

stefan.r’s picture

FileSize
517 bytes
2.94 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,647 pass(es). View

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

stefan.r’s picture

Status: Needs work » Needs review

The last submitted patch, 88: interdiff-61-86.patch, failed testing.

josephdpurcell’s picture

Reviewing this now.

josephdpurcell’s picture

Issue summary: View changes

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

josephdpurcell’s picture

Issue summary: View changes

Fix link to comment in description.

josephdpurcell’s picture

Issue summary: View changes

Add manual testing steps to the summary.

josephdpurcell’s picture

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

Status: Needs review » Needs work

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.

stefan.r’s picture

FileSize
2.99 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,539 pass(es). View
726 bytes

Is the drush problem in scope here?

stefan.r’s picture

Status: Needs work » Needs review
stefan.r’s picture

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

xjm’s picture

Thanks @josephdpurcell and @stefan.r!

For Drush, let's file a pull request against Drush and just reference it here.

I updated the remaining tasks in the summary.

stefan.r’s picture

josephdpurcell’s picture

Reviewing and manually testing now.

josephdpurcell’s picture

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

Issue summary: View changes
Issue tags: -Needs issue summary update

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

josephdpurcell’s picture

Issue summary: View changes

Marking remaining task as done.

josephdpurcell’s picture

Status: Needs review » Reviewed & tested by the community

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.

josephdpurcell’s picture

Status: Reviewed & tested by the community » Needs work

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.

josephdpurcell’s picture

Issue summary: View changes

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

josephdpurcell’s picture

Status: Needs work » Needs review
FileSize
6.39 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,534 pass(es), 1 fail(s), and 0 exception(s). View
josephdpurcell’s picture

stefan.r’s picture

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
stefan.r’s picture

Status: Needs review » Needs work

The last submitted patch, 110: 2296929-110.patch, failed testing.

josephdpurcell’s picture

Status: Needs work » Needs review
FileSize
24.06 KB
44.04 KB
47.67 KB
59.59 KB
68.4 KB
36.03 KB
66.71 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Setup environment: Test cancelled by admin prior to completion. View
2.68 KB
  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






josephdpurcell’s picture

FileSize
66.68 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 35,346 pass(es), 1,021 fail(s), and 796 exception(s). View
1.03 KB

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

This patch puts back the real code.

The last submitted patch, 115: 2296929-115.patch, failed testing.

stefan.r’s picture

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?

josephdpurcell’s picture

FileSize
3.75 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,185 pass(es). View
978 bytes

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

josephdpurcell’s picture

And the screenshots:

BEFORE:

AFTER:

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

The last submitted patch, 116: 2296929-116.patch, failed testing.

stefan.r’s picture

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

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • alexpott committed d1bdf99 on 8.0.x
    Issue #2296929 by josephdpurcell, codemonkie, stefan.r, YesCT, alexpott...

  • alexpott committed 25e0bb9 on 8.0.x
    Revert "Issue #2296929 by josephdpurcell, codemonkie, stefan.r, YesCT,...
alexpott’s picture

Status: Fixed » Needs work

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.

josephdpurcell’s picture

Status: Needs work » Needs review
FileSize
4.95 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,209 pass(es), 1 fail(s), and 0 exception(s). View
1.94 KB

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.

josephdpurcell’s picture

FileSize
2.99 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,204 pass(es). View
978 bytes

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.

The last submitted patch, 128: 2296929-128.patch, failed testing.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC, and back to #98

stefan.r’s picture

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

xjm’s picture

Status: Reviewed & tested by the community » Needs work

This looks great! The one thing we may still need is to explicitly add a #weight key to each to ensure they are in order, similarly to @Wim Leers' feedback in #2505499-48: Explicitly support render arrays in hook_requirements().

akalata’s picture

Status: Needs work » Needs review
FileSize
3.08 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,232 pass(es). View
1.48 KB
joelpittet’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/system.install
@@ -440,17 +440,22 @@
+      '#weight' => 2
...
+      '#weight' => 3

Nit: Need some ending commas on these array items.

akalata’s picture

Status: Needs work » Needs review
FileSize
3.09 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,230 pass(es). View
789 bytes

I knew it!

David_Rothstein’s picture

+    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...

joelpittet’s picture

@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.

joelpittet’s picture

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

David_Rothstein’s picture

@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 :)

joelpittet’s picture

@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.

joelpittet’s picture

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?

joelpittet’s picture

FileSize
2.03 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,182 pass(es). View

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.

stefan.r’s picture

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

Status: Needs review » Needs work
+++ 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.

alexpott’s picture

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.

xjm’s picture

+++ 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)))));
+    $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)))));

One of the !cron didn't get converted to @cron here also. But is @alexpott saying in #146 that we shouldn't change that line anyway?

I'm still kinda concerned about the unnecessary escaping-followed-by-filtering, but it is less verbose. So if people think that's the way to go, OK... but we're trading one problematic pattern for another. =/

stefan.r’s picture

Status: Needs work » Needs review
FileSize
957 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,208 pass(es). View

voila

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

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

stefan.r’s picture

FileSize
2.96 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,177 pass(es). View
stefan.r’s picture

FileSize
2.98 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,197 pass(es). View
stefan.r’s picture

FileSize
3 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,199 pass(es). View
stefan.r’s picture

FileSize
1.97 KB
alexpott’s picture

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

xjm’s picture

So #136 is not actually really more verbose than HEAD. All the same lines are there in HEAD, and actually in places that are less scannable because they are spread out.

I do share the reservations in general about the DX of joining multiple t(), and @effulgentsia also shared concerns about the use of #prefix and #suffix as patterns that get abused, but I feel like arrays of the strings are the best solution so far because the render API is an API we already have, that developers from D7 already know how to use and will need to use elsewhere in D8, and it has minimal side effects.

#2554073: Allow #markup to be an array of strings and join them safely is interesting but we already have multiple examples of using render arrays in HEAD, so I don't think it makes sense to block this particular patch on it. If we want we can improve the DX later.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

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.

joelpittet’s picture

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.

joelpittet’s picture

@xjm can you explain #147

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

Especially the part where the first pattern is problematic.

joelpittet’s picture

Status: Reviewed & tested by the community » Needs review

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.

  • xjm committed 1b4e1ee on 8.0.x
    Issue #2296929 by stefan.r, josephdpurcell, codemonkie, YesCT, alexpott...
xjm’s picture

Status: Needs review » Fixed

@joelpittet, my comment was about the fact that using #markup => t() . ' ' . t() . ... unnecessarily filters text that has already been escaped, so that the patch changes what happens in HEAD.

I think we can explore fixing the DX in #2554073: Allow #markup to be an array of strings and join them safely. Since the pattern does already exist in HEAD, and this issue is the child of a critical, I am going to commit this patch and we can evaluate those instances in a non-release-blocking followup.

Thanks everyone for your patience on this issue. It's definitely something of the canary in the coal mine, as @effulgentsia put it, in terms of highlighting deficiencies in our best practices. I think though we need to move on from this particular instance and revisit this and other usages in non-release-blocking issues.

xjm’s picture

Okay I actually did not commit this from Needs review... I had been typing the comment for the past 90 minutes and discussing it with @joelpittet at the same time in IRC.

However, I reiterate that the pattern exists elsewhere in HEAD and the difference in this function is not worth blocking release on. This issue was also committed once before because of a hunk that was added to the scope that has since been removed and also fixed elsewhere.

joelpittet’s picture

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.

Status: Fixed » Closed (fixed)

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

hass’s picture