Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Follow-up to #2509898: Additional uncaught exception thrown while handling exception after service changes, the following call to sprintf
in core/includes/error.inc
is invalid. In fact the format string uses SafeMarkup::format()
style placeholders instead of sprintf
(i.e. %s
, %d
).
error_log(sprintf('Failed to log error: %type: !message in %function (line %line of %file).', $error['%type'], $error['%function'], $error['%line'], $error['%file']));
Proposed resolution
Change the line to something like:
error_log(sprintf('Failed to log error: %s: %s in %s (line %d of %s).', $error['%type'], $error['!message'], $error['%function'], $error['%line'], $error['%file']));
Or perhaps better:
error_log(strtr('Failed to log error: %type: !message in %function (line %line of %file).', $error);
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#12 | invalid_format_string-2540438-12.patch | 5.31 KB | znerol |
#12 | interdiff.txt | 1.58 KB | znerol |
#8 | TEST-ONLY-invalid_format_string-2540438-8.patch | 4.68 KB | znerol |
#8 | invalid_format_string-2540438-8.patch | 5.42 KB | znerol |
#4 | invalid_format_string-2540438-4.patch | 755 bytes | timmillwood |
Comments
Comment #1
znerol CreditAttribution: znerol commentedComment #2
znerol CreditAttribution: znerol commentedComment #3
timmillwoodSomewhat related to #2514044: Do not use SafeMarkup::format in exceptions
Comment #4
timmillwoodAdding patch for review
Comment #5
dawehnerUps. I think some form of test would be nice!
Comment #6
timmillwoodI'm not sure how to test this, we'd need to call
_drupal_log_error
make the line\Drupal::logger('php')->log($error['severity_level'], '%type: !message in %function (line %line of %file).', $error);
in that function throw an exception so we can assert error_log.Comment #7
dawehnerOh I see the problem, there is no active logger in those tests. Once you install the syslog module you'll see the following error message:
Comment #8
znerol CreditAttribution: znerol commentedtest patch === interdiff.
Comment #10
timmillwood@Znerol - Thanks for the patch.
Test with patch from #4 passes, and test without the patch from #4 fails, as it should. So... RTBC!
Comment #11
dawehnerNice test!
That works fine of course as well
Comment #12
znerol CreditAttribution: znerol commentedTBH, I really prefer
strtr
oversprintf
here. It is shorter and we can simply reuse theSafeMarkup
placeholders already present in$error
.The flags are not necessary, hence do not specify them.
Comment #13
timmillwoodPersonally I prefer sprintf, but not sure why. So happy with the changes in #12, and still RTBC.
Comment #14
alexpottstrtr() makes sense to me because that is what SafeMarkup::format does.
Committed 9845999 and pushed to 8.0.x. Thanks!