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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

znerol’s picture

Issue summary: View changes
znerol’s picture

Issue summary: View changes
timmillwood’s picture

timmillwood’s picture

Status: Active » Needs review
FileSize
755 bytes

Adding patch for review

dawehner’s picture

Issue tags: +Needs tests

Ups. I think some form of test would be nice!

timmillwood’s picture

Status: Needs review » Needs work

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

dawehner’s picture

Oh 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:

[29-Jul-2015 14:13:12 Europe/Berlin] Failed to log error: PDOException: SQLSTATE[HY000] [1044] Access denied for user ''@'localhost' to database 'dev_d8' in Drupal\Core\DependencyInjection\Container\prod\service_container_prod_17922066->getDatabaseService() (line 2169 of /Users/dawehner/www/d8/sites/simpletest/718875/files/php/service_container/service_container_prod_17922066/d7fecede21ed82914c847615e8e0205a93398ef7e7520b556d81a9be75ed0f2a.php).
znerol’s picture

test patch === interdiff.

Status: Needs review » Needs work

The last submitted patch, 8: TEST-ONLY-invalid_format_string-2540438-8.patch, failed testing.

timmillwood’s picture

Status: Needs work » Reviewed & tested by the community

@Znerol - Thanks for the patch.

Test with patch from #4 passes, and test without the patch from #4 fails, as it should. So... RTBC!

dawehner’s picture

Nice test!

+++ b/core/modules/system/tests/modules/error_service_test/error_service_test.services.yml
@@ -6,3 +6,8 @@ services:
+  logger.broken:
+    class: Drupal\error_service_test\Logger\TestLog
+    tags:
+      - { name: logger }
+      - { name: backend_overridable }

That works fine of course as well

znerol’s picture

  1. +++ b/core/includes/errors.inc
    @@ -159,7 +159,7 @@ function _drupal_log_error($error, $fatal = FALSE) {
    +      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']));
    

    TBH, I really prefer strtr over sprintf here. It is shorter and we can simply reuse the SafeMarkup placeholders already present in $error.

  2. +++ b/core/modules/system/src/Tests/System/UncaughtExceptionTest.php
    @@ -186,6 +186,35 @@ public function testLostDatabaseConnection() {
    +    $errors = file(\Drupal::root() . '/' . $this->siteDirectory . '/error.log', FILE_IGNORE_NEW_LINES | FILE_SKIP_EMPTY_LINES);
    

    The flags are not necessary, hence do not specify them.

timmillwood’s picture

Personally I prefer sprintf, but not sure why. So happy with the changes in #12, and still RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

strtr() makes sense to me because that is what SafeMarkup::format does.

Committed 9845999 and pushed to 8.0.x. Thanks!

  • alexpott committed 9845999 on 8.0.x
    Issue #2540438 by znerol, timmillwood: Invalid format string when...

Status: Fixed » Closed (fixed)

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