Problem/Motivation

Since t() replacements were introduced around ten years ago, there has been a fallthrough for unrecognised placeholders to %placeholder.

I don't think anyone has realised this was there, or why it was added, at any point in the ten years since.

      case '%':
      default:
        // Escaped and placeholder.
        if (!SafeMarkup::isSafe($value)) {
          $value = Html::escape($value);
        }
        $args [$key] = '<em class="placeholder">' . $value . '</em>';
        break;

Proposed resolution

Trigger an error when another placeholder is used, and stop replacing the replacement value.

For array keys that start with an alphabetic character we skip the triggering of the error as they likely weren't intended to be placeholder variables in the first place and core won't ever add any placeholders that start with an alphabetic character.

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch created an issue. See original summary.

joshi.rohit100’s picture

Status: Active » Needs review
FileSize
562 bytes
catch’s picture

Status: Needs review » Reviewed & tested by the community

RTBC pending the bot.

I think we should look at throwing an exception for unrecognised placeholders, but that can be its own follow-up. The default may never have been reached, so this is just removing dead code as it is.

plach’s picture

I think this needs a CR

stefan.r’s picture

IMO we should throw an exception or assert if it's an unrecognized placeholder - why not just do it in this issue?

Note this would fail on !placeholders as well.

dawehner’s picture

Sounds like we need at least a test, maybe an exception being thrown?

xjm’s picture

Priority: Normal » Major
Issue tags: +Needs change record, +Needs tests

Yep, let's get a CR and also test coverage for the actual expected behavior of having an invalid placeholder in the $args.

borisson_’s picture

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

Writing a test.

borisson_’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.5 KB
1.78 KB
borisson_’s picture

Addressing @dawehner's feedback.

The last submitted patch, 9: remove_default-2575703-9.patch, failed testing.

The last submitted patch, 9: remove_default-2575703-9.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 10: remove_default-2575703-10.patch, failed testing.

The last submitted patch, 10: remove_default-2575703-10.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
461 bytes
2.24 KB

Status: Needs review » Needs work

The last submitted patch, 15: remove_default-2575703-15.patch, failed testing.

The last submitted patch, 15: remove_default-2575703-15.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
2.75 KB
3.46 KB

Patch uses trigger_error over new \InvalidArgumentException.

Status: Needs review » Needs work

The last submitted patch, 18: remove_default-2575703-17.patch, failed testing.

The last submitted patch, 18: remove_default-2575703-17.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
3.87 KB

Reroll attached.

Status: Needs review » Needs work

The last submitted patch, 21: remove_default-2575703-21.patch, failed testing.

The last submitted patch, 21: remove_default-2575703-21.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
5.34 KB
2.03 KB

This is going to be fun. Tracked the cache tag related fails down to "List entity_test entities referencing the given entity" being passed to that.

The string goes through TitleResolver, and that passes any raw variable as @something, !something *and* %something to t(). Absolutely crazy :)

Status: Needs review » Needs work

The last submitted patch, 24: remove_default-2575703-24.patch, failed testing.

The last submitted patch, 24: remove_default-2575703-24.patch, failed testing.

stefan.r’s picture

Any reason for this to be an E_USER_ERROR rather than an exception? I think either is fine, but just wondering...

dawehner’s picture

Any reason for this to be an E_USER_ERROR rather than an exception? I think either is fine, but just wondering...

The problem is, we are inside a __toString() call here, potentially.

stefan.r’s picture

Well we have ToStringTrait which allows us to catch exceptions - shouldn't we just use that with all the __toStrings?

borisson_’s picture

Fixes SafeMarkupTest (The only remaining failure after @Berdir's earlier patch).

There are still a couple of cases were there's an invalid placeholder: Invalid placeholder: severity_level.

I'm looking into the remaining failures.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
    @@ -147,14 +158,8 @@ protected static function placeholderFormat($string, array $args) {
    diff --git a/core/lib/Drupal/Core/Controller/TitleResolver.php b/core/lib/Drupal/Core/Controller/TitleResolver.php
    
    diff --git a/core/lib/Drupal/Core/Controller/TitleResolver.php b/core/lib/Drupal/Core/Controller/TitleResolver.php
    index 4fe90a6..9adff8c 100644
    
    index 4fe90a6..9adff8c 100644
    --- a/core/lib/Drupal/Core/Controller/TitleResolver.php
    
    --- a/core/lib/Drupal/Core/Controller/TitleResolver.php
    +++ b/core/lib/Drupal/Core/Controller/TitleResolver.php
    
    +++ b/core/lib/Drupal/Core/Controller/TitleResolver.php
    +++ b/core/lib/Drupal/Core/Controller/TitleResolver.php
    @@ -60,7 +60,6 @@ public function getTitle(Request $request, Route $route) {
    
    @@ -60,7 +60,6 @@ public function getTitle(Request $request, Route $route) {
           if (($raw_parameters = $request->attributes->get('_raw_variables'))) {
             foreach ($raw_parameters->all() as $key => $value) {
               $args['@' . $key] = $value;
    -          $args['!' . $key] = $value;
               $args['%' . $key] = $value;
             }
    

    I'm 100% sure the TitleResolverTest will fail

  2. +++ b/core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
    @@ -192,6 +206,41 @@ function providerCheckPlain() {
    +    // We set a custom error handler because of https://github.com/sebastianbergmann/phpunit/issues/487
    +    set_error_handler([$this, 'errorHandler']);
    +    // We want this to trigger an error.
    +    $error = SafeMarkup::format('Broken placeholder: ~placeholder', ['~placeholder' => 'broken'])->__toString();
    +    restore_error_handler();
    

    urgs, again!

Status: Needs review » Needs work

The last submitted patch, 30: remove_default-2575703-30.patch, failed testing.

stefan.r’s picture

Just so we don't have to do the trigger_error trickery, can we use ToStringTrait on FormattableString instead (we already do on TranslatableString) and throw an exception when other placeholders are used? That'll make it fatal properly...

The last submitted patch, 30: remove_default-2575703-30.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
5.92 KB

Patch no longer applied.

Status: Needs review » Needs work

The last submitted patch, 35: remove_default-2575703-35.patch, failed testing.

The last submitted patch, 35: remove_default-2575703-35.patch, failed testing.

joelpittet’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue tags: -Novice +Needs issue summary update

@borisson_ this doesn't seem to be doing what the title says. This looks like it needs an issue summary update.

Either way this unfortunately didn't make it into D8 rc so I'll move it to 8.1.x for hopes it can get in as a feature.

Berdir’s picture

Even if we don't remove the fallback, there are plenty of bugfixes in there, like removing the ! placeholder from TitleResolver and some other places that we should and can get in as a bugfix to 8.0.x I think.

That said, actually removing the fallback doesn't seem like something that we can still do in 8.x anyway. So maybe it's easier to continue with this patch to fix the test fails and in the end just get it in without actually removing the fallback.

effulgentsia’s picture

Version: 8.1.x-dev » 8.0.x-dev
Issue tags: +rc target triage

Not removing the fallback means we pretty much can't introduce any new placeholder prefixes at any time during 8.x without breaking theoretical BC. On the other hand, removing it during RC would likely not be very disruptive (not sure if there's a way to easily query for how many contrib modules make use of the fallback). So I think it's worth considering still doing this, so tagging for committer triage.

xjm’s picture

Issue tags: -rc target triage +rc target

@alexpott and I also agreed with making this an RC target.

The last submitted patch, 35: remove_default-2575703-35.patch, failed testing.

stefan.r’s picture

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

Updated issue summary. Are we OK to just throw an exception when another placeholder is used, or do we fail silently by removing the "default:" line?

joelpittet’s picture

Assigned: borisson_ » Unassigned

I'm not opposed throwing an error. Would rather default to @.

stefan.r’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
5.78 KB
1.1 KB

As we have a few stray array keys ("placeholders") that start with alphabetic characters which are causing tests to fail, and as contrib may have the same problem, maybe let's just silently pass on those? The only characters we want to "reserve" are the non-alphabetic ones anyway.

As to #33, this late into the release cycle a trigger_error may be more appropriate than an exception (which would fatal).

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

That seems to be a nice middle ground @stefan.r thanks.

stefan.r’s picture

stefan.r’s picture

+++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
@@ -232,7 +232,13 @@
+          // We do not trigger an error for placeholder that start with an

s/placeholder/placeholders/

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record

This approach seems a sensible compromise and definitely will help people upgrade modules find the new : placeholder. Committed 35ed46b and pushed to 8.0.x. Thanks!

  • alexpott committed 35ed46b on 8.0.x
    Issue #2575703 by borisson_, stefan.r, Berdir, joshi.rohit100: Remove...
Wim Leers’s picture

This caused:

wim.leers at wimleers-acquia in ~/Work/drupal-tres on 8.0.x*
$ di tres
Password:
You are about to DROP all tables in your 'tres' database. Do you want to continue? (y/n): y
Starting Drupal installation. This takes a while. Consider using the --notify global option. [ok]
Installation complete.  User name: root  User password: root                                 [ok]
Congratulations, you installed Drupal!                                                       [status]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !type FormattableMarkup.php:240                                         [error]
Invalid placeholder: !type FormattableMarkup.php:240                                         [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !type FormattableMarkup.php:240                                         [error]
Invalid placeholder: !type FormattableMarkup.php:240                                         [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !type FormattableMarkup.php:240                                         [error]
Invalid placeholder: !type FormattableMarkup.php:240                                         [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !type FormattableMarkup.php:240                                         [error]
Invalid placeholder: !type FormattableMarkup.php:240                                         [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !default FormattableMarkup.php:240                                      [error]
Invalid placeholder: !extensions FormattableMarkup.php:240                                   [error]

After updating drush to latest (composer global update), which is 1858a83, this still happens.

stefan.r’s picture

@WimLeers does https://github.com/drush-ops/drush/pull/1740 fix this for you?

Wim Leers’s picture

It did! Providing feedback there.

@stefan.r++

joelpittet’s picture

This seems to have caused issues because we've not finished this other issue removing !placeholder.

#2571953: Remove !placeholder in syslog module

joelpittet’s picture

Oh that was drush as well for me too... maybe this would be better if we got more context on that error message?

Add the string to the message:

trigger_error('Invalid placeholder: ' . $key . ' in string: ' . $string, E_USER_ERROR);
alexpott’s picture

@joelpittet - good idea - let's do that in a new issue.

stefan.r’s picture

Status: Fixed » Closed (fixed)

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