Follow-up to #2514044: Do not use SafeMarkup::format in exceptions
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).
Use sprintf()/concatentation/magic strings 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 '#'.");
Fix core
None
None
None
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
alexpottComment #5
dawehnerLooks fine for me!
Comment #6
alexpottSo trigger_error is not automatically escaped by the error handler unlike exceptions and debug messages. I'm going to separate out those changes. And handle them in yet another issue once #2501319: Remove SafeMarkup::set() in Error::renderExceptionSafe() and _drupal_log_error() and improve backtrace formatting consistency in _drupal_log_error(), Error::renderExceptionSafe(), and DefaultExceptionSubscriber::onHtml () lands.
Comment #8
sagar ramgade commentedThe patch attached removes sprintf usages with single and double quotes for exceptions.
Comment #10
dawehnerLet's just tackle instances which use
Safemarkup::formatComment #19
jeroentSince SafeMarkup no longer exists, I believe this issue is outdated?
Comment #20
jeroentComment #21
alexpottWell we still use
\Drupal\Component\Render\FormattableMarkupin exceptions which was the replacement forSafeMarkup... for example see\Drupal\Core\Config\UnmetDependenciesException::create()Comment #27
quietone commentedComment #29
quietone commentedJust found this one. My searches didn't turn up other possible instances to change.
Comment #30
dcam commentedThe current changes look good, but I found several other instances of
FormattableMarkupobjects being used in exceptions. Can you double-check my findings and verify whether these should be changed by the MR too?core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.phpline 96core/lib/Drupal/Core/Entity/ContentEntityBase.phplines 855, 866, and 877Comment #31
mondrakeA maybe a PHPStan rule could be developed to prevent this to happen in the future?
Comment #33
longwaveAdded a PHPStan rule.
Comment #34
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #35
longwaveThe PHPStan rule found some more cases, which is encouraging.