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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Sounds fine to me :)

alexpott’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
5.77 KB

Patch 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.

Status: Needs review » Needs work

The last submitted patch, 3: 2514044.3.patch, failed testing.

alexpott’s picture

Title: Use sprintf and not SafeMarkup::format in exceptions » Do not use SafeMarkup::format in exceptions
Issue summary: View changes

Perhaps we shouldn't even bother with the sprintf() - it just makes things harder to read for little gain. See https://r.je/sprintf.html

// 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 '#'.");

Re performance of sprintf, concatenation and magic expansion... http://3v4l.org/sc4sl

alexpott’s picture

I'm not sure about:

throw new \InvalidArgumentException("The user-entered string $user_input must begin with a '/', '?', or '#'.");

versus

throw new \InvalidArgumentException('The user-entered string ' . $user_input . " must begin with a '/', '?', or '#'.");

One thing that is nice with the second example is that it is clear where the variable is.

xjm’s picture

xjm’s picture

I think double quotes are fine and I find them more readable than the concatenation, but I also think it's irrelevant for the issue?

Gábor Hojtsy’s picture

@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 :)

dawehner’s picture

I think double quotes are fine and I find them more readable than the concatenation, but I also think it's irrelevant for the issue?

I 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.

xjm’s picture

@pfrenssen referenced https://www.drupal.org/coding-standards#quotes -- which says:

With that caveat in mind, single quote strings should be used by default. Their use is recommended except in two cases:

Deliberate in-line variable interpolation, e.g. "<h2>$header</h2>".

but also:

Drupal does not have a hard standard for the use of single quotes vs. double quotes. Where possible, keep consistency within each module, and respect the personal style of other developers.

So. :) Whichevs.

(Edited for markup.)

alexpott’s picture

Status: Needs work » Needs review
FileSize
110.72 KB

So 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.

dawehner’s picture

FileSize
126.6 KB
51.12 KB

Fun.

Status: Needs review » Needs work

The last submitted patch, 13: 2514044-13.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
130.57 KB
4.22 KB

Fixed the remaining failures. I think this patch is good to go!

timmillwood’s picture

+++ b/core/modules/config/src/Tests/ConfigImporterTest.php
@@ -295,8 +295,8 @@ function testSecondaryWriteSecondaryFirst() {
+    $message = SafeMarkup::format("'config_test' entity with ID '@name' already exists", array('@name' => 'secondary'));
+    $this->assertEqual($logs[0], SafeMarkup::format('Unexpected error during import with operation @op for @name: !message.', array('@op' => 'create', '@name' => $name_primary, '!message' => $message)));

Shouldn't the SafeMarkup be removed here too?

dawehner’s picture

Shouldn't the SafeMarkup be removed here too?

Good catch!

I think this is an out of scope change, this issue is about exceptions.

Status: Needs review » Needs work

The last submitted patch, 17: 2514044-17.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
131.61 KB
2.1 KB

Okay 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.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

Sounds good to me!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

There are exceptions that use format_string()

alexpott’s picture

Status: Needs work » Needs review
FileSize
168.27 KB
12.07 KB
timmillwood’s picture

+++ b/core/modules/locale/src/StringDatabaseStorage.php
@@ -199,9 +199,7 @@ public function delete($string) {
+      throw new StringStorageException('The string cannot be deleted because it lacks some key fields: ' . $string->getString());

@@ -483,9 +481,7 @@ protected function dbStringInsert($string) {
+      throw new StringStorageException('The string cannot be saved: ' . $string->getString());

@@ -516,9 +512,7 @@ protected function dbStringUpdate($string) {
+      throw new StringStorageException('The string cannot be updated: ' . $string->getString());

Could these be changed to "...: {$string->getString()}"?

alexpott’s picture

@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.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

ok, RTBC'ing again then.

stefan.r’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: 2514044-2.22.patch, failed testing.

dawehner’s picture

Issue tags: +Needs reroll

.

stefan.r’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
168.27 KB
timmillwood’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

@stefan.r looks like #29 has parts of a different patch in it too.

stefan.r’s picture

@timmillwood, where exactly? Maybe those were in #22 already?

timmillwood’s picture

@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?

stefan.r’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
28.86 KB
142.85 KB

Seems 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

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

@stefan.r thanks - I just recreated the same patch and there were no differences... sorry everyone.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 33: 2514044-32.patch, failed testing.

timmillwood’s picture

Issue tags: +Needs reroll
stefan.r’s picture

Status: Needs work » Needs review
FileSize
2.55 KB
142.94 KB
timmillwood’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

Thanks @stefan.r

stefan.r’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
142.85 KB
570 bytes

typo

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

Good spot!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed f862962 on 8.0.x
    Issue #2514044 by dawehner, stefan.r, alexpott: Do not use SafeMarkup::...
alexpott’s picture

I've updated the docs on https://www.drupal.org/node/608166. Should we do a CR?

sasanikolic’s picture

This 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

alexpott’s picture

catch’s picture

For change record I think we need to update and flesh out https://www.drupal.org/node/2302363

Status: Fixed » Closed (fixed)

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