Switch from deprecation notice to warning for non-standard placeholders in FormattableMarkup::placeholderFormat()

Just re-titling, since we don't support these as such already.

Should probably be an assert (+ no-op) rather than an exception?

+++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
@@ -231,17 +231,12 @@ protected static function placeholderFormat($string, array $args) {
-            trigger_error('Invalid placeholder (' . $key . ') in string: ' . $string, E_USER_ERROR);
+          // Deprecate suuport for random variables that won't be replaced.
+          if (ctype_alpha($key[0]) && strpos($string, $key) === FALSE) {
+            @trigger_error(sprintf('Support for keys without a placeholder prefix is deprecated in Drupal 9.1.0 and will be removed in Drupal 10.0.0. Invalid placeholder (%s) with string: "%s"', $key, $string), E_USER_DEPRECATED);
           }

I agree with deprecating this for Drupal 10, but reckon we could still deprecate it in 8.9 - it's deprecating a code path, not deprecating actual code, and code doing this is already buggy.

There is a small typo in the following comment (suuport instead of support).

+          // Deprecate suuport for random variables that won't be replaced.
+          if (ctype_alpha($key[0]) && strpos($string, $key) === FALSE) {
+            @trigger_error(sprintf('Support for keys without a placeholder prefix is deprecated in Drupal 9.1.0 and will be removed in Drupal 10.0.0. Invalid placeholder (%s) with string: "%s"', $key, $string), E_USER_DEPRECATED);

Should it postponed on bug fix? #3109795: Entity plural label context is not set as expected

This is disruptive, so it would be at least a minor-only change. Since 8.9.x and 9.0.x are now in beta, I'm moving this to 9.1.x. Thanks!

I wonder if there's a way to do this less disruptively with a deprecation for the minor?

I wonder if there's a way to do this less disruptively with a deprecation for the minor?

That's sort of what the patch does. The patch can be hard to read in places but summarizing the test changes we can see the actual effect of the change.

|First Character    |Matches in string?  | Test value    | Change               |
|-------------------|--------------------|---------------|----------------------|
|Non-alpha          |Always              |'-placeholder' |ERROR => WARNING      |
|Alpha              |Matches             |'placeholder'  |DEPRECATED => WARNING |
|Alpha              |No matches          |'foo'          |DEPRECATED            |

So this patch is doing 3 things but only 2 should matter.
1) Removing a deprecation. Seems like this should have been in 9.0 but it didn't have a timeline or standard deprecation testing. Probably because of its age.
2) Deprecating a previously unhandled error condition showing mistakes was silently thrown away previously. This is a DX improvement as seen in #16 but maybe isn't allowed in 9.0.

If we really wanted to target 9.0 we could just split the new deprecation and replace it with a TODO or just backport this without the deprecation. A follow up at this point would be one line and a test but I'm not sure it is worth it since we are so late in the 9.0.

1. +++ b/core/includes/errors.inc
   @@ -149,6 +149,11 @@ function error_displayable($error = NULL) {
    
   +  // Remove 'severity_level' as is not a valid replacement for t().
   +  $severity = $error['severity_level'];
   +  unset($error['severity_level']);
   +  $error['%severity_level'] = $severity;
   +
   

   I feel like I'm missing something, but:

   1. Where is $error['%severity_level'] used?

2. +++ b/core/includes/errors.inc
   @@ -167,7 +172,7 @@ function _drupal_log_error($error, $fatal = FALSE) {
          // Provide the PHP backtrace to logger implementations.
   -      \Drupal::logger('php')->log($error['severity_level'], '%type: @message in %function (line %line of %file) @backtrace_string.', $error + ['backtrace' => $backtrace]);
   +      \Drupal::logger('php')->log($severity, '%type: @message in %function (line %line of %file) @backtrace_string.', $error + ['backtrace' => $backtrace, 'severity_level' => $severity]);
        }
        catch (\Exception $e) {
   

   2. Aren't we now passing $error['%severity_level'] and $error['severity_level'] to the ->log() call?

yeah good catch... catch. I think without looking too close it just made sense to keep it. I dug through this for a while and error is 99% a local variable and its not used locally and when we hand it off we add the variable back so in the bigger picture as shown in 2) setting '%severity_level' doesn't seem needed. If a logger wants to add the log severity to the message for some reason it has all it needs to make that happen.

Looking at the diff in #27.1 in a different context, I don't think we actually pass this to t(), it looks like its all strtr and FormatableMarkup right? We can just say "... as it is not a valid replacement."

#29 looks better.

Committed e85462d and pushed to 9.1.x. Thanks!

I think it's fine for this to just land in 9.1.x and not be backported.

Closed #2860659: Broken argument replacements should trigger an E_USER_ERROR, not E_USER_DEPRECATED as a duplicate, adding credit.

This is still causing problems. Please see #3173103: False positives when identifying what is a placeholder, for deprecation error. In short, the PSR-3 $context parameter holds more than just placeholders, but Drupal generates a warning if the array keys in $context don't conform to the "placeholder" requirements.