Problem/Motivation

Whilst reading Drupal Planet I happened upon https://chromatichq.com/blog/drupal-code-standards-t-function. It makes an interesting recommendation about how to not escape HTML when using TranslatableMarkup.

So now you might be wondering what to do in Drupal 8 if you do not want to escape your text at all, now that the !variable placeholder is gone. There is a way! In the placeholderFormat() function, there’s a default case. Let’s take a look at the code:

default:
        // We do not trigger an error for placeholder that start with an
        // alphabetic character.
        if (!ctype_alpha($key[0])) {
          // We trigger an error as we may want to introduce new placeholders
          // in the future without breaking backward compatibility.
          trigger_error('Invalid placeholder (' . $key . ') in string: ' . $string, E_USER_ERROR);
        }
        break;
    }

So if we use a placeholder that starts with an alphabetic character, it won’t trigger an error, but it also won’t trigger any actions. You can use any alphabetic character(s) you like for your dummy placeholder. Something easily identifiable, like TT is a good idea - and comment your code!

The problem here is that since this often used in MarkupInterface object's twig won't auto-escape and if the input comes from users we have a problem.

Proposed resolution

tbd

Remaining tasks

User interface changes

None

API changes

Likely that there will be a behaviour change.

Data model changes

None

CommentFileSizeAuthor
#29 2807705-29.patch7.33 KBalexpott
#29 27-29-interdiff.txt1.84 KBalexpott
#27 2807705-27.patch7.1 KBalexpott
#27 10-27-interdiff.txt6.64 KBalexpott
#10 2807705-10.patch2.66 KBalexpott
#5 2807705-5.patch1.88 KBalexpott
#4 2807705-4.patch1.45 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 
          // We do not trigger an error for placeholder that start with an
          // alphabetic character.

I'm not sure why we did that.

alexpott’s picture

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

I think this was a total accident introduced by #2575703: Remove default fall-through from PlaceholderTrait::placeholderFormat(). My mistake I should have caught this. It seems #2575703-45: Remove default fall-through from PlaceholderTrait::placeholderFormat() was where it got introduced.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Active » Needs review
FileSize
2807705-4.patch1.45 KB

Here's proof. Now we need to decide what to do.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2807705-5.patch1.88 KB

Let's see what breaks in core if we just remove the if.

Status: Needs review » Needs work

The last submitted patch, 5: 2807705-5.patch, failed testing.

alexpott’s picture

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

I think doing #5 might be unpalatable for existing sites so the question is what are the options... I think they are:

  1. Call PlainTextOutput::renderFromHtml() - will strip all html
  2. Call Html::escape() - will escape all html
  3. Call Xss::filter() - will strip most html

Also I think this action should be consistent with the other escaping done here - ie only occur if the value is not an object that implements MarkupInterface.

alexpott’s picture

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

Of course we could also just to the Drupal 7 thing and escape + placeholder. I think that is the best thing to do. Since it also has been the behaviour for most of the D8 life-cycle

alexpott’s picture

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

Created #2807743: Switch from deprecation notice to warning for non-standard placeholders in FormattableMarkup::placeholderFormat() to address this properly in D9.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
2807705-10.patch2.66 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Related issues: +#2575703: Remove default fall-through from PlaceholderTrait::placeholderFormat()
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
@@ -227,11 +227,16 @@ protected static function placeholderFormat($string, array $args) {
+          $args[$key] = '<em class="placeholder">' . static::placeholderEscape($value) . '</em>';

I would have expected that we should not do the em placeholder, BUT, actually that is the behaviour of Drupal 7, so let's not change that.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

I think we should open another issue to trigger E_USER_DEPRECATED for an 8.x minor release, this looks fine for a patch release.

alexpott credited aburke626.

alexpott’s picture

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

Adding @aburke626 to contribution since it was their blog post that reported the issue.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented

Adding @dawehner to contribution for reviewing.

  • effulgentsia committed 8477ed5 on 8.3.x 
    Issue #2807705 by alexpott, dawehner, aburke626: FormattableMarkup::...
effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +rc target triage

Pushed to 8.3.x. Setting to "patch (to be ported)" for cherry picking to 8.2.x in a patch release, per #13.

However, also adding the "rc target triage" tag to consider cherry picking this to 8.2.x prior to 8.2.0. It is after all a behavior change, including the addition of the <em> wrapper, so IMO, better to break sites relying on the old behavior as part of the 8.1 to 8.2 upgrade than during an 8.2 patch release.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented

In the issue summary of #2575703: Remove default fall-through from PlaceholderTrait::placeholderFormat() we said we'd "stop replacing the replacement value", essentially ignoring placeholders that start with an alphabetic character. Possibly we found some code where the arguments array had arguments that were not intended to be placeholders altogether?

Per #4 it looks like we didn't do so however -- I guess we intended to put an unset($args[$key]); in there (seems #7 also forgot to list that as an option). Maybe we should do that here instead? Yes it will break strings for people using the workaround mentioned in OP but these "alphabetical" placeholders were never supported/documented, and now we're breaking the previous behavior anyway by escaping it and wrapping it in em tags. We're also adding yet another placeholder type (albeit undocumented), which people might similarly start to use when they really should be using %placeholder.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
Version: 8.2.x-dev » 8.3.x-dev
Status: Patch (to be ported) » Needs work

I guess we intended to put an unset($args[$key]); in there (seems #7 also forgot to list that as an option). Maybe we should do that here instead?

+1 to this from me. I agree that that is no worse of a BC break than #17, and an acceptable thing to do in a minor. Setting to NW and 8.3.x for it to see if that idea gets traction. If it doesn't, we can reset status and version for deciding on what to do for 8.2.x with what's already been committed to 8.3.

alexpott’s picture

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

I think the breaking change of not having something display is worse for sites than still doing the D7 behaviour. So I'm in favour of doing #10 in D8 and 8.2.x and then following up with #2807743: Switch from deprecation notice to warning for non-standard placeholders in FormattableMarkup::placeholderFormat() in D9 as the hard breaking change.

alexpott’s picture

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

now we're breaking the previous behavior anyway by escaping it and wrapping it in em tags. We're also adding yet another placeholder type (albeit undocumented), which people might similarly start to use when they really should be using %placeholder.

Yes but this has been the D7 behaviour and the D8 behaviour for everything except the last 11 months.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

For me I think I'd prefer the hard break (i.e. don't replace those placeholders). This is a bug that some people might be relying on, it's not part of the public facing API, so fixing that bug seems fine.

alexpott’s picture

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

I'm going to revert #17 since it obvious we don't quite have consensus yet and more discussion is required.

  • alexpott committed f506915 on 8.3.x 
    Revert "Issue #2807705 by alexpott, dawehner, aburke626:...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Suggestion from irc after talking with alexpott and stefan_r:

1. Don't replace invalid placeholders.

2. trigger_error('...', E_USER_DEPRECATED) if both the argument uses an invalid placeholder AND the string has the invalid placeholder in it. This will hopefully mean core still passes - since we add arbitrary arguments with error messages.

3. The trigger_error shouldn't use Symfony's @ suppression trick, because rather than being an API we previously expected people to use, this is one that was never supposed to exist at all and breaks our security model.

If #2 results in core tests fails we have other problems though.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
10-27-interdiff.txt6.64 KB
2807705-27.patch7.1 KB

Here's the new approach.

I've moved stuff from SafeMarkupTest to FormattableMarkupTest because it makes discovering the right test easier. And adding test functionality of the deprecated message to SafeMarkupTest felt really wrong. But we should file a followup to move more of the SafeMarkupTest to the correct tests whilst leaving some testing of the deprecated functionality.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
@@ -227,11 +227,18 @@ protected static function placeholderFormat($string, array $args) {
+          elseif (strpos($string, $key)) {

Needs a !== FALSE. Could also put the placeholder first in the test coverage to catch that.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
27-29-interdiff.txt1.84 KB
2807705-29.patch7.33 KB

Nice catch catch. And alexpott--

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue tags: -rc target triage +rc target

Given the security nature of this and the fact that what ever we do is a behaviour change I think we need to address this before 8.2.0 therefore marking as an 'rc target'

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented

+1 to #29

We also discussed how we want to do a CR that explains the change in behavior (even if this is about an unsupported/undocumented feature), as well as contact @aburke626 asking her to update the article

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
Status: Needs review » Reviewed & tested by the community

#29 looks great to me.

We also discussed how we want to do a CR that explains the change in behavior

Do you think a new CR is needed, or just an update to https://www.drupal.org/node/2605274?

alexpott’s picture

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

I think updating https://www.drupal.org/node/2605274 is an excellent idea.

  • catch committed 652ab4a on 8.3.x 
    Issue #2807705 by alexpott, dawehner, aburke626: FormattableMarkup::...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.3.x-dev » 8.2.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs change record updates

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

Agreed on updating the change record. It's not even loading for me at the moment though.

  • catch committed 78fbc0f on 8.2.x 
    Issue #2807705 by alexpott, dawehner, aburke626: FormattableMarkup::...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +8.2.0 release notes
xjm’s picture

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

Added a paragraph to the CR to cover the new change:
https://www.drupal.org/node/2605274/revisions/view/10116811/10118643

Could use review.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Looks good to me.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: -Needs change record updates

Status: Fixed » Closed (fixed)

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