Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#45 | interdiff.txt | 1.1 KB | stefan.r |
#45 | 2575703-48.patch | 5.78 KB | stefan.r |
#35 | remove_default-2575703-35.patch | 5.92 KB | borisson_ |
Comments
Comment #2
joshi.rohit100Comment #3
catchRTBC 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.
Comment #4
plachI think this needs a CR
Comment #5
stefan.r CreditAttribution: stefan.r commentedIMO 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.
Comment #6
dawehnerSounds like we need at least a test, maybe an exception being thrown?
Comment #7
xjmYep, let's get a CR and also test coverage for the actual expected behavior of having an invalid placeholder in the
$args
.Comment #8
borisson_Writing a test.
Comment #9
borisson_Comment #10
borisson_Addressing @dawehner's feedback.
Comment #15
borisson_Comment #18
borisson_Patch uses trigger_error over new \InvalidArgumentException.
Comment #21
borisson_Reroll attached.
Comment #24
BerdirThis 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 :)
Comment #27
stefan.r CreditAttribution: stefan.r commentedAny reason for this to be an E_USER_ERROR rather than an exception? I think either is fine, but just wondering...
Comment #28
dawehnerThe problem is, we are inside a
__toString()
call here, potentially.Comment #29
stefan.r CreditAttribution: stefan.r commentedWell we have ToStringTrait which allows us to catch exceptions - shouldn't we just use that with all the __toStrings?
Comment #30
borisson_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.
Comment #31
dawehnerI'm 100% sure the TitleResolverTest will fail
urgs, again!
Comment #33
stefan.r CreditAttribution: stefan.r commentedJust 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...
Comment #35
borisson_Patch no longer applied.
Comment #38
joelpittet@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.
Comment #39
BerdirEven 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.
Comment #40
effulgentsia CreditAttribution: effulgentsia at Acquia commentedNot 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.
Comment #41
xjm@alexpott and I also agreed with making this an RC target.
Comment #43
stefan.r CreditAttribution: stefan.r commentedUpdated 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?
Comment #44
joelpittetI'm not opposed throwing an error. Would rather default to @.
Comment #45
stefan.r CreditAttribution: stefan.r commentedAs 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).
Comment #46
joelpittetThat seems to be a nice middle ground @stefan.r thanks.
Comment #47
stefan.r CreditAttribution: stefan.r commenteddraft CR at https://www.drupal.org/node/2605274
Comment #48
stefan.r CreditAttribution: stefan.r commenteds/placeholder/placeholders/
Comment #49
alexpottThis 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!Comment #51
Wim LeersThis caused:
After updating drush to latest (
composer global update
), which is1858a83
, this still happens.Comment #52
stefan.r CreditAttribution: stefan.r commented@WimLeers does https://github.com/drush-ops/drush/pull/1740 fix this for you?
Comment #53
Wim LeersIt did! Providing feedback there.
@stefan.r++
Comment #54
joelpittetThis seems to have caused issues because we've not finished this other issue removing !placeholder.
#2571953: Remove !placeholder in syslog module
Comment #55
joelpittetOh 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:
Comment #56
alexpott@joelpittet - good idea - let's do that in a new issue.
Comment #57
stefan.r CreditAttribution: stefan.r commentedcreated #2608054: Give more context in invalid placeholder error message PlaceholderTrait::placeholderFormat()