Problem/Motivation

See #2566503: [meta] Replace remaining !placeholder for Non-URL HTML outputs only

format_size() is can be inserted into translated strings using !placeholder.

Note we also need to check for objects that wrap it like MigrateExecutable...

  /**
   * Wraps format_size()
   *
   * @return string
   *   The formatted size.
   */
  protected function formatSize($size) {
    return format_size($size);
  }

Proposed resolution

It is totally safe to replace ! with @ here. Since format_size() is supposed to return output from t(). However it's implementation completely breaks the safe list. That is being fixed in #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list since after that it will return a TranslationWrapper.

There are many places in core that already use an @placeholder or %placeholder. For example:

drupal_set_message(t('There is not enough memory available to PHP to change this theme\'s color scheme. You need at least %size more. Check the <a href="@url">PHP documentation</a> for more information.', array('%size' => format_size($usage + $required - $size), '@url' => 'http://www.php.net/manual/ini.core.php#ini.sect.resource-limits')), 'error');

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#8 interdiff.txt2.15 KBjoelpittet
#8 replace_remaining-2568793-8.patch3.25 KBjoelpittet
#5 2568793-5.patch3.1 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
Issue summary: View changes
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
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Active » Needs review
FileSize
2568793-5.patch3.1 KB

Given that the @placeholder and %placeholder usage with respect to format_size() far far outweighs !placeholder usage I think it is not worth adding any test coverage here. A simple review with --color-words should suffice.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Yeah this looks alright

alexpott’s picture

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

This is part of the critical meta.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +SafeMarkup, +D8 Accelerate
FileSize
replace_remaining-2568793-8.patch3.25 KB
interdiff.txt2.15 KB 
+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -462,10 +462,10 @@ protected function memoryExceeded() {
+        $this->t('Memory usage is @usage (!pct% of limit @limit), reclaiming memory.',
           array('!pct' => round($pct_memory*100),

@@ -473,19 +473,19 @@ protected function memoryExceeded() {
+          $this->t('Memory usage is now @usage (!pct% of limit @limit), not enough reclaimed, starting new batch',
             array('!pct' => round($pct_memory*100),
...
+          $this->t('Memory usage is now @usage (!pct% of limit @limit), reclaimed enough, continuing',
             array('!pct' => round($pct_memory*100),

Converted this !pct value here too as it's going to likely cause conflicts anyways we look at it and the scope impact is minimal. Also these are the only instances of !pct in all of core.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Reviewed & tested by the community

Looks good, verified with git diff --color-words that only the placeholder character was changed.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented

I manually tested the first hunk. It checks out!

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented

Markup before and after is identical (Twig debug comments included for context):

Before:

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'file_upload_help' -->
<!-- BEGIN OUTPUT from 'core/themes/classy/templates/content-edit/file-upload-help.html.twig' -->
One file only.<br />2 MB limit.<br />Allowed types: png gif jpg jpeg.

<!-- END OUTPUT from 'core/themes/classy/templates/content-edit/file-upload-help.html.twig' -->

After:

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'file_upload_help' -->
<!-- BEGIN OUTPUT from 'core/themes/classy/templates/content-edit/file-upload-help.html.twig' -->
One file only.<br />2 MB limit.<br />Allowed types: png gif jpg jpeg.

<!-- END OUTPUT from 'core/themes/classy/templates/content-edit/file-upload-help.html.twig' -->
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

Thanks @Cottser!

Committed and pushed to 8.0.x.

  • xjm committed 4f43541 on 8.0.x 
    Issue #2568793 by joelpittet, alexpott, Cottser, klausi: Replace...

Status: Fixed » Closed (fixed)

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