The !variable place holder for the proper formatting of strings is no longer valid as per !placeholder removed from t() and format_string(), and is causing errors. To further illustrate, it has been removed from the documentation.

This is an example of a produced error:

User error: Invalid placeholder (!count) in string: !amount every !count months in Drupal\Component\Render\FormattableMarkup::placeholderFormat() (line 233 of core/lib/Drupal/Component/Render/FormattableMarkup.php).

Here's how to find all of the instances:

colan@snake[Mon 13 12:48]% grep -rn "'\!" .                                                                                                                 
./src/Form/RecurlyRedeemCouponForm.php:194:        '!coupon' => $this->recurlyFormatter->formatCoupon($coupon, $form_state->getValue(['coupon_currency'])),
./src/RecurlyEntityOperations.php:106:          '!title' => \Drupal::l($entity->label(), $entity->toUrl()),
./src/RecurlyFormatManager.php:139:      '!count' => $html ? '<span class="plan-count">' . SafeMarkup::checkPlain($count) . '<span>' : SafeMarkup::checkPlain($count),
./src/RecurlyFormatManager.php:140:      '!amount' => $html ? '<span class="plan-amount">' . $amount . '</span>' : $amount,
./src/RecurlyFormatManager.php:145:          return t('!amount per day', $replacements);
./src/RecurlyFormatManager.php:148:          return t('!amount per month', $replacements);
./src/RecurlyFormatManager.php:152:      return t('!amount per week', $replacements);
./src/RecurlyFormatManager.php:155:      return t('!amount per year', $replacements);
./src/RecurlyFormatManager.php:160:          return t('!amount every !count days', $replacements);
./src/RecurlyFormatManager.php:163:          return t('!amount every !count months', $replacements);
CommentFileSizeAuthor
#11 variable_placeholder-2747935-11.patch6.4 KBadamzimmermann
#6 interdiff-2747935-5-6.txt1.97 KBcolan
#6 recurly-replace_bang_variable_placeholders-2747935-6.patch6.25 KBcolan
#5 recurly-replace_bang_variable_placeholders-2747935-5.patch5.67 KBcolan
#5 interdiff-2747935-3-5.txt2.01 KBcolan
#3 recurly-replace_bang_variable_placeholders-2747935-3.patch4.47 KBcolan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

colan created an issue. See original summary.

markdorison’s picture

markdorison
he/him
CreditAttribution: markdorison at Chromatic commented
Related issues: +#2641566: Invalid placeholder in strings
colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
CreditAttribution: colan at Colan Schwartz Consulting commented
Status: Active » Needs review
FileSize
recurly-replace_bang_variable_placeholders-2747935-3.patch4.47 KB

I got all of the RecurlyFormatManager bits working and tested with this patch, but some additional testing is needed for the first two instances. I'm not that far along in my set-up to test these.

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
CreditAttribution: colan at Colan Schwartz Consulting commented
Status: Needs review » Needs work

The preformatted currency amounts are showing up with HTML tags as t() is escaping them. Working on including a fix...

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
CreditAttribution: colan at Colan Schwartz Consulting commented
Assigned: colan » Unassigned
Priority: Normal » Major
Status: Needs work » Needs review
FileSize
interdiff-2747935-3-5.txt2.01 KB
recurly-replace_bang_variable_placeholders-2747935-5.patch5.67 KB

Here's another patch, but I'm not crazy about this solution. Because the formatter methods are adding their own markup (span tags) and there's no longer a natural means by which to render unsanitized text, the fix here is doing the following:

  • Unescaping the text that gets sent through t(), and
  • Unescaping the text that gets rendering in the Twig template.

I think we need to refactor how we're formatting currency amounts and price intervals (formatCurrency() and formatPriceInterval() respectively) to avoid doing this sort of thing. I'm not much of a themer, but here are some options (please add/edit/etc.):

  1. Don't do anything else; use the solution in this patch.
  2. Drop all of the formatting span tags within the output for now. We can worry about it when we get a feature request to make the sub-elements stylable.
  3. Move the formatters to the preprocessor. I'm not sure if we're really supposed to be putting markup in there either, but at least it's closer to the theme. We'll only need to escape the text once this way, in the template.
  4. Move all of the markup into the Twig template. This appears to be the cleanest solution. We'd have to create more variables with the sub-elements of the currency amounts and intervals, but this isn't a big deal.

If someone else who's front-end inclined could take a look into this, I'd really appreciate it.

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
CreditAttribution: colan at Colan Schwartz Consulting commented
FileSize
recurly-replace_bang_variable_placeholders-2747935-6.patch6.25 KB
interdiff-2747935-5-6.txt1.97 KB

Added some missing fixes for various interval formats.

aburke626’s picture

aburke626
she/her
Primary language English
Location Harveys Lake, PA, US
CreditAttribution: aburke626 at Chromatic commented

This patch looks solid to me - and I agree that a refactoring would be a good idea. Do you mind creating a new issue(s) for that as a starting point, and we can try to get some front-enders on it?

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
CreditAttribution: colan at Colan Schwartz Consulting commented
Title: "!variable" placeholder still being used for string formatting » "!variable" placeholder used for string formatting; needs refactoring

@aburke626: Thanks for looking into this.

How about simply taking over this issue? Feel free to rename it and/or change / add to the description.

I'm not expecting that we commit my patch above so I'm not sure we need separate issues. In other words, if this ticket is staying open, let's use it.

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
CreditAttribution: colan at Colan Schwartz Consulting commented
Status: Needs review » Needs work
adamzimmermann’s picture

adamzimmermann CreditAttribution: adamzimmermann at Chromatic commented
Assigned: Unassigned » adamzimmermann

I have a patch nearly complete for this that integrates with a template as suggested above. I want to do some additional testing next week, then I'll post it.

adamzimmermann’s picture

adamzimmermann CreditAttribution: adamzimmermann at Chromatic commented
Status: Needs work » Needs review
FileSize
variable_placeholder-2747935-11.patch6.4 KB

This removes all ! placeholders, and retains existing functionality to the best of my knowledge.

I did pass FALSE to formatCoupon in RecurlyRedeemCouponForm.php to avoid dealing with HTML in the t() function there. I don't think losing that price markup is too big of a deal though.

adamzimmermann’s picture

adamzimmermann CreditAttribution: adamzimmermann at Chromatic commented
Assigned: adamzimmermann » Unassigned
markdorison’s picture

markdorison
he/him
CreditAttribution: markdorison at Chromatic commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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