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
We are escaping exception messages when we are creating exceptions. If the exception message is never displayed or used this wastes valuable time escaping and consume static memory in SafeMarkup::set().
Amusingly all exception messages are escaped again in Error::decodeException()
so if it is displayed then there is double escaping. (Hence this issue having a bug status).
Proposed resolution
Use sprintf() to munge strings together when creating exceptions.
// In \Drupal\Component\Utility\UrlHelper
throw new \InvalidArgumentException(SafeMarkup::format('A path was passed when a fully qualified domain was expected.'));
// Should be
throw new \InvalidArgumentException('A path was passed when a fully qualified domain was expected.');
// In \Drupal\Core\Url
throw new \InvalidArgumentException(SafeMarkup::format("The user-entered string @user_input must begin with a '/', '?', or '#'.", ['@user_input' => $user_input]));
// Should be
throw new \InvalidArgumentException("The user-entered string $user_input must begin with a '/', '?', or '#'.");
Remaining tasks
Agree approach
Change documentation https://www.drupal.org/node/608166
Fix core
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#39 | interdiff-37-38.txt | 570 bytes | stefan.r |
#39 | 2514044-38.patch | 142.85 KB | stefan.r |
#33 | interdiff-29-32.txt | 28.86 KB | stefan.r |
#29 | 2514044-29.patch | 168.27 KB | stefan.r |
#22 | 19-22-interdiff.txt | 12.07 KB | alexpott |
Comments
Comment #1
alexpottComment #2
Gábor HojtsySounds fine to me :)
Comment #3
alexpottPatch attached illustrates that using SafeMarkup::format in exception messages will result in double escaping - thereby making them harder to read that they should be.
This patch will have a single failure caused by double escaping of exception messages.
Comment #5
alexpottPerhaps we shouldn't even bother with the
sprintf()
- it just makes things harder to read for little gain. See https://r.je/sprintf.htmlRe performance of sprintf, concatenation and magic expansion... http://3v4l.org/sc4sl
Comment #6
alexpottI'm not sure about:
versus
One thing that is nice with the second example is that it is clear where the variable is.
Comment #7
xjmComment #8
xjmI think double quotes are fine and I find them more readable than the concatenation, but I also think it's irrelevant for the issue?
Comment #9
Gábor Hojtsy@xjm: I think we also want to update the coding styles here, so what we suggest matters. Also for the patch to fix the core uses its best to agree :)
Comment #10
dawehnerI agree with that statement. You still see the actual string, especially useful for debugging purposes. In case you see an exception you want to make it easy to go back to the root
which is easer when you have variable substitution.
Comment #11
xjm@pfrenssen referenced https://www.drupal.org/coding-standards#quotes -- which says:
but also:
So. :) Whichevs.
(Edited for markup.)
Comment #12
alexpottSo here's a patch that rips out SafeMarkup from exceptions and just tries to do the most readable thing with respect to
"
and'
. I think we might want to consider having a standard that wraps anything that is variable with an exception in a single quote.Comment #13
dawehnerFun.
Comment #15
dawehnerFixed the remaining failures. I think this patch is good to go!
Comment #16
timmillwoodShouldn't the
SafeMarkup
be removed here too?Comment #17
dawehnerGood catch!
I think this is an out of scope change, this issue is about exceptions.
Comment #19
dawehnerOkay that was stupid, the exception changed.
@timmillwood
I think getting rid of SafeMarkup::format() is wrong here, because we want to optimize for the best reviewablity. So changing this here, would be a different context change.
Comment #20
timmillwoodSounds good to me!
Comment #21
alexpottThere are exceptions that use
format_string()
Comment #22
alexpottComment #23
timmillwoodCould these be changed to
"...: {$string->getString()}"
?Comment #24
alexpott@timmillwood yes but there didn't seem to be any advantage in that. I don't think our standards should be so strict as to define how to concatenate strings in a exception message.
Comment #25
timmillwoodok, RTBC'ing again then.
Comment #26
stefan.r CreditAttribution: stefan.r commentedComment #28
dawehner.
Comment #29
stefan.r CreditAttribution: stefan.r commentedComment #30
timmillwood@stefan.r looks like #29 has parts of a different patch in it too.
Comment #31
stefan.r CreditAttribution: stefan.r commented@timmillwood, where exactly? Maybe those were in #22 already?
Comment #32
timmillwood@stefan.r - ah, yes, it is in #22 too, how did I miss that?
@alexpott - that's not supposed to be in there, is it?
Comment #33
stefan.r CreditAttribution: stefan.r commentedSeems the other patch crept into #22 but the interdiff was correct. Here's a re-roll of #19 with the interdiff in #22 applied to it
Comment #34
alexpott@stefan.r thanks - I just recreated the same patch and there were no differences... sorry everyone.
Comment #36
timmillwoodComment #37
stefan.r CreditAttribution: stefan.r commentedComment #38
timmillwoodThanks @stefan.r
Comment #39
stefan.r CreditAttribution: stefan.r commentedtypo
Comment #40
timmillwoodGood spot!
Comment #41
catchCommitted/pushed to 8.0.x, thanks!
Comment #43
alexpottI've updated the docs on https://www.drupal.org/node/608166. Should we do a CR?
Comment #44
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedThis caused many test fails to some contrib modules. Here is the followup issue with the fix. #2547357: $error in case of a views query exception
Comment #45
alexpottCreated #2552163: Do not use FormattableMarkup in exceptions, trigger_error, and debug (the second pass)
Comment #46
catchFor change record I think we need to update and flesh out https://www.drupal.org/node/2302363