Problem/Motivation

Joining strings and preserving safeness id extremely painful in Drupal. We need a way to do this sanely with minor changes to the current API and preserving performance.

Proposed resolution

Support arrays of strings in #markup and filter or escape according to using #safe_strategy and #allowed_tags.

$render_array = [
  '#markup => [
    t('Lorem'),
    ', '
    t('ipsum').
  ],
];
print $renderer->render($render_array); // prints "Lorem, ipsum."

All non-safe strings (t()'s output is safe) are by default admin filtered. Safe strings are considered safe and left unfiltered or escaped. This behaviour can be changed by using #safe_strategy and #allowed_tags.

Remaining tasks

Agree this is worthwhile.
Review
Commit

User interface changes

None.

API changes

No change only addition.

Data model changes

None.

CommentFileSizeAuthor
#22 alex_render_perf_test3.txt1.52 KBalexpott
#21 2554073.20.patch5.07 KBalexpott
#21 12-20-interdiff.txt3.17 KBalexpott
#18 render_perf_test_other_twig.txt1.47 KBstefan.r
#14 render_perf_test.php_.txt1.45 KBstefan.r
#12 2554073.12.patch4.66 KBalexpott
#12 2-12-interdiff.txt1.7 KBalexpott
#2 2554073.2.patch4 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Active » Needs review
FileSize
2554073.2.patch4 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

So for example the code in #2296929: Remove system_requirements() SafeMarkup::set() use could become...

    if ($severity != REQUIREMENT_INFO) {
      $requirements['cron']['description'][#markup] = [
        t('Cron has not run recently.'),
        ' ',
        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']),
        ' '
      ];
    }
    $requirements['cron']['description'][#markup][] = t('You can <a href="@cron">run cron manually</a>.', ['@cron' => \Drupal::url('system.run_cron')]);
    $requirements['cron']['description'][#markup][] = '<br/>';
    $requirements['cron']['description'][#markup][] =  t('To run cron from outside the site, go to <a href="@cron">@cron</a>', ['@cron' => \Drupal::url('system.cron', ['key' => \Drupal::state()->get('system.cron_key'), ['absolute' => TRUE]])]);
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue summary: View changes
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

FWIW I think this is a good idea.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

A note regarding the beta eligibility of this: This is a followup to a critical (the whole SafeMarkup::set() removal meta) and therefore I think it can be accepted during the beta provided the impact outweighs the disruption if we decide to go forward with it.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Related issues: +#2296929: Remove system_requirements() SafeMarkup::set() use, +#2501683: Remove SafeMarkup::set in _update_message_text(), +#2505499: Explicitly support render arrays in hook_requirements()

Also relevant (in addition to the related issues): the HelpController. And some others I haven't tracked down yet.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Related issues: +#2501639: Remove SafeMarkup::set in drupal_check_module(), +#2501947: Remove SafeMarkup::set in ViewUI::renderPreview(), +#2451607: Remove call to SafeMarkup::set() from node_requirements()
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Related issues: +#2549791: Remove SafeMarkup::xssFilter() and provide ability to define #markup escaping strategy and what tags to filter
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

The example in #3 looks like it should be a single t() string to me. In some languages, you might have 'cron' in the first sentence, but then no pronouns at all in later sentences because the context has already been set (i.e. "To run manually from outside the site.." - no 'cron', no 'it'). Each sentence as a t() string prevents a translator from doing that if they wanted to. Whether they want to probably depends on the language.

xjm and alexpott pointed out two things in irc:

1. We already do this in 7.x - this is true, but doesn't mean it's necessarily correct.

2. Conditional strings make it tricky - three short strings where one is conditional vs. two long strings that are identical except for the conditional.

Concern for me is that t() . t() . t() is a workaround, and render arrays of #markup is a workaround, but this would provide a proper way to do this, whereas we might just want to encourage longer t() strings for these cases.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2-12-interdiff.txt1.7 KB
2554073.12.patch4.66 KB

We can also use this to support separators... in a way more efficient manor.

With the patch attached and the patch in #2505931: Remove SafeMarkup::set in ViewListBuilder and using the script https://gist.github.com/alexpott/8b3d9b3a55e87919ec20 with opcache enabled and through a browser.

  • #markup render array in 0.00047707557678223 seconds
  • Twig render array in 0.002061128616333 seconds
  • Implode + Escape in 0.00012302398681641 seconds
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Also perhaps I didn't give the best example by using t() since this has become a debate about what is best for translators. The result of t() is just one thing that we're adding together and we need to remain safe:

  • inline lists eg #2505931: Remove SafeMarkup::set in ViewListBuilder
  • SafeMarkup::format('@onefish @twofish') eg. 
    $cell_content = \Drupal::l(SafeMarkup::format('@cell_content@image', array('@cell_content' => $cell_content, '@image' => $image)), new Url('<current>', [], [
      'attributes' => array('title' => $title),
      'query' => array_merge($ts['query'], array(
        'sort' => $ts['sort'],
        'order' => $cell_content,
      )),
    ]));
stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
FileSize
render_perf_test.php_.txt1.45 KB

@alexpott I'm not sure about that benchmark, can you try attached?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@stefan.r

// Do an unrelated render to set up stuff...
$renderer = \Drupal::service('renderer');
$markup = ['#markup' => 'ignore'];
$renderer->renderPlain($twig_render_array);

You are priming the inline list - this is not right.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

But also... we should use the same escaping strategy!... nice spot @stefan.r

With the performance script attached:

  • Twig render array in 0.0041379928588867 seconds
  • #markup render array in 0.00072407722473145 seconds
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
alex_render_perf_test.php.txt1.52 KB
stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
FileSize
render_perf_test_other_twig.txt1.47 KB

Well I like the idea of being able to do ['#separator' => ', ', '#markup' => $arr], that was just to see how much faster the patch in this issue was faster in such case. If we just prime an empty markup array we initialize the theme during the benchmark so that's not exactly right either.

What if we prime another twig template which should be a more realistic comparison? (see attached)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@stefan.r you still need to set the #safe_strategy in the markup array... when you do that and prime with another twig template you get:

  • Twig render array in 0.0027940273284912 seconds
  • #markup render array in 0.0012531280517578 seconds

But #markup is going to be primed 99.9999999% of the time.

If you prime both another twig template and #markup...

  • Twig render array in 0.0029680728912354 seconds
  • #markup render array in 0.00069379806518555 seconds
stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented

About the safe_strategy, yes -- I hadn't seen your posts in #16/#17 yet.

Assuming that latest benchmark is the most realistic of all a 2ms improvement is significant...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
12-20-interdiff.txt3.17 KB
2554073.20.patch5.07 KB

Here's a way that we can optimise the escaping/filtering and only do it once... which means that the importance of setting the escaping strategy disappears.

Results with the perf script attached:

  • Twig render array in 0.0034749507904053 seconds
  • #markup render array in 0.0011019706726074 seconds
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
alex_render_perf_test3.txt1.52 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

I like the idea of being able to do ['#separator' => ', ', '#markup' => $arr]

+1

#19: So, Twig render array 2–3 milliseconds, #markup render array 0.6–0.7 milliseconds. Quite big difference.
#21: and here 3.5 ms vs 1.1. Why did #markup render array get roughly twice as slow in this new iteration?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

I posted on #2505931: Remove SafeMarkup::set in ViewListBuilder - what Views is doing currently also feels wrong to me.

I'd be tempted to say we should use the 'array of render arrays with markup' workaround for these cases, that's not a functional regression from HEAD since we're already handling those strings wrong for RTL in those cases.

That leaves us with some ugly code, but sometimes ugly code for doing ugly things is what you want. Then we can look at a nice inline option for item lists and/or making concatenated t() strings into longer single strings (or find out my RTL concern is misplaced and do this patch) in a minor release.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented

Do we need to revive this?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Version: 8.2.x-dev » 8.3.x-dev
Status: Needs review » Needs work
Issue tags: +Needs reroll
anish.a’s picture

anish.a CreditAttribution: anish.a at Axelerant commented

The core/lib/Drupal/Core/Render/Element/Markup.php file is removed on the following commit

commit 00d1bdbe8f6b4eab8d781beb38e95912d548378d
Author: Nathaniel Catchpole <catch@35733.no-reply.drupal.org>
Date:   Sat Aug 29 00:10:40 2015 +0100

    Issue #2555931 by alexpott, pwolanin, joelpittet: Add #plain_text to escape text in render arrays

This is not a trivial reroll. May have to rewrite the patch.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

arunkumark’s picture

arunkumark
Location Coimbatore
CreditAttribution: arunkumark as a volunteer and at DrupalPartners for Innoppl Technologies Pvt. Ltd commented
Issue tags: -Needs reroll +rewrite

The file does not appear further, So ned to re-write so removing tag Needs reroll and adding rewrite

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.