If you have code like this:
drupal_set_message(t('My name is !name.', ['!name' => 'David']));
Drupal will not perform the replacement, and instead will output "My name is !name" to the screen. It will also trigger an error to notify developers about the problem.
However, if you have code like this:
drupal_set_message(t('My name is NAME.', ['NAME' => 'David']));
Drupal will still not perform the replacement (it will output "My name is NAME"). But instead of triggering an error, it will trigger an E_USER_DEPRECATED.
I don't think that makes sense. If it's broken code (not merely deprecated code) it should be an error just like the first case.
This issue is relatively minor, because it turns out Drupal logs/displays E_USER_DEPRECATED as if it were an error or a warning (so even if your development site is only configured to display errors/warnings to the screen, you'll probably still see it). That's presumably a bug though, and we shouldn't rely on it.
Also, my main interest in this issue is that it seems to be what the documentation in https://www.drupal.org/core/deprecation#how-unintended is based on. I think that documentation should be fixed as well as a followup to this issue - E_USER_DEPRECATED should only be used if something is deprecated but still works.
Comment | File | Size | Author |
---|---|---|---|
#5 | 2860659-5.patch | 2.81 KB | David_Rothstein |
#2 | 2860659-1.patch | 1.41 KB | David_Rothstein |
Comments
Comment #2
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedHere is a patch.
Comment #3
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedA small side effect of this patch is that if you have code like this:
(which obviously won't lead to @name being replaced), then before the patch, an error is triggered, but after the patch, it isn't.
I think the post-patch behavior makes sense (unlike the examples in the issue summary, this isn't code that ever worked in previous versions of Drupal or that anyone should ever have expected to work - it is just a flat-out typo). But if for some reason we want to preserve that behavior, the patch could be changed to do so.
Comment #5
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedQuick fix - some tests needed to be updated for this change too.
Comment #6
GoZ CreditAttribution: GoZ at Centarro commentedComment #7
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis functionality already has tests - see above.
Comment #9
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThe patch still applies (and tests still pass) on the 8.5.x branch.
Reviews?
Comment #11
borisson_I agree, this makes sense! The patch still applies. I ran
FormattableMarkupTest
locally and it passes.Comment #12
alexpottWe original it was a deprecation - see #2575703: Remove default fall-through from PlaceholderTrait::placeholderFormat() - yes that issue also changed the behaviour so it makes it not really deprecated. I think it should have used
E_USER_WARNING
rather than deprecated. Like the user thinks the replacement is going to work but it's not. But it did use to work in D7.If we want to make it an
E_USER_ERROR
for both instances - and that is probably fine I think we should remove theif
and always error if we get here.This code is definitely wonky...
won't error but...
will... I don't really see why. Since we don't to any replacements on default anymore why not just error.
Comment #13
alexpottAlso
drupal_set_message(t('My name is @name.', ['!name' => 'David']));
should error since!
is not supported.drupal_set_message(t('My name is @name.', [':name' => 'David']));
shouldn't error because:
is supported. And it does not now and it doesn't with whatever approach we take.Comment #23
quietone CreditAttribution: quietone at PreviousNext commentedThis was fixed as part of #2807743: Switch from deprecation notice to warning for non-standard placeholders in FormattableMarkup::placeholderFormat(). I am closing as a duplicate and moving credit.