Ensure that entries are written to watchdog table

Good idea, let's see how we can implement that in the new database layer.

This issue will not go anywhere without confirmation that this issue still exists in current Drupal 7.x and without a patch against a current version.

What version of D6 still has this issue?

BTW: Your last patch failed. Without a valid patch this issue won't go anywhere.

Please attached your patch and mark this ticket as "needs review". The automated patch review system will tell you if the patch is acceptable.

If this is against Drupal 6.22 (which it should be), the version needs to be changed on the issue.

Your patch has been "ignored". Try changing the version to 6.x-dev and resubmitting the patch.

Still applies to 8.x, which uses the default connection for watchdog messages.

Let's not confuse the system. If this patch is needed and applied cleanly to 8.x-dev, lets open a new issue in that queue.

Patches go into the highest version of drupal they are applicable to first, tmk, which would be 8.x-dev. This is the appropriate issue for it.

#15: dedicated_watchdog_connection-298768.patch queued for re-testing.

Retesting against latest HEAD since it has been over a year.

1. +++ b/core/modules/dblog/src/Logger/DbLog.php
   @@ -60,21 +66,41 @@ public function log($level, $message, array $context = array()) {
   +        // Open a dedicated connection for logging to ensure that errors that
   +        // messed up the default connection are still logged.
   

   I think it would be great to describe the error in the issue summary. ...

2. +++ b/core/modules/dblog/src/Tests/ConnectionFailureTest.php
   @@ -0,0 +1,48 @@
   +    // MySQL erros like "1153 – Got a packet bigger than ‘max_allowed_packet’
   +    // bytes" or "1100 - Table 'xyz' was not locked with LOCK TABLES" lead to a
   +    // database connection unusable for further request. All further request
   +    // will result in an "2006 - MySQL server had gone away" error. As a
   +    // consequence it's impossible to use this connection to log the causing
   +    // initial error itself. Using Database::closeConnection() we simulate such
   +    // a corrupted connection. In this case dblog has to establish a different
   +    // connection by itself to be able to write the log entry.
   

   Basically something like this?

RTBC, this looks great how simple after all it is to setup a new DB connection! :)

Issue summary is good, needs a beta evaluation.

--

I wonder if simply doing what you do in the test would not be easier? (closeConnection, openConnection)

Though that would restore DB access for all of Drupal, which could lead to severe errors in itself as it might have been missing out on data, then trying to write it to cache.

So this is fine as is - also while dependency injection is nicer, in this case it is fine to go via the lowest subsystem level possible as in the mean time e.g. the container might be corrupted already, or other things have happened.

I love how dependency-injection overall makes this issue (quite) easy and clean.

Thanks!

1. +++ b/core/modules/dblog/src/Logger/DbLog.php
   @@ -60,21 +66,52 @@ public function log($level, $message, array $context = array()) {
   +      // When running Drupal on MySQL or Maria DB you can run in several errors
   

   MariaDB, run into

2. +++ b/core/modules/dblog/src/Tests/ConnectionFailureTest.php
   @@ -0,0 +1,48 @@
   + * Tests loging of connection failures.
   

   logging

3. +++ b/core/modules/dblog/src/Tests/ConnectionFailureTest.php
   @@ -0,0 +1,48 @@
   +   * Tests loging of connection failures.
   

   logging

4. +++ b/core/modules/dblog/src/Tests/ConnectionFailureTest.php
   @@ -0,0 +1,48 @@
   +    // MySQL erros like "1153 – Got a packet bigger than ‘max_allowed_packet’
   

   errors

5. +++ b/core/modules/dblog/src/Tests/ConnectionFailureTest.php
   @@ -0,0 +1,48 @@
   +    // database connection unusable for further request. All further request
   

   requests

We forgot a second "request" :)

Gave the code a quick look if you think this looks good now, I think this can go back to RTBC.

Fixing a 81 character line

I just wanted to test the patch with some real use case and excahged
Database::closeConnection()
with

    try {
      $max_allowed_packet = db_query("SELECT @@global.max_allowed_packet")->fetchField();
      $long_name = str_repeat("a", $max_allowed_packet + 10);

      Database::getConnection('default')
        ->insert('watchdog')
        ->fields(array(
          'uid' => 20,
          'type' => 'test',
          'message' => $long_name,
          'variables' => '',
          'severity' => '',
          'link' => '',
          'location' => '',
          'referer' => '',
          'hostname' => '',
          'timestamp' => 12345,
        ))
        ->execute();
    }
    catch (\Exception $e) {}

My point was to actually break the connection. Now the test fails locally with

exception 'PDOException' with message 'SQLSTATE[HY000]: General error: 2006 MySQL server has gone away' in core/lib/Drupal/Core/Database/Statement.php:61

Next exception 'Drupal\Core\Database\DatabaseExceptionWrapper' with message 'SQLSTATE[HY000]: General error: 2006 MySQL server has gone away: SELECT table_name FROM information_schema.tables WHERE  (table_schema = :db_condition_placeholder_0) AND (table_name LIKE :db_condition_placeholder_1 ESCAPE '\\') ; Array
(
    [:db_condition_placeholder_0] => dev_d8
    [:db_condition_placeholder_1] => simpletest466931%
)
' in core/lib/Drupal/Core/Database/Connection.php:609

Am I missing something or doing the test wrong?

@mkalkbrenner:
only uncaught exceptions get logged. That is why I do the test in a try catch block, so that the exception does not reach the ExceptionLoggingSubscriber.

@mkalkbrenner
you are right. In this case the patch and the test look good to me.
@mkalkbrenner++ very good work!

Requeueing as tests failed with copy(/var/lib/drupaltestbot/sites/default/files/checkout/sites/simpletest/996777/settings.php): failed to open stream: No such file or directorycopy('/var/lib/drupaltestbot/sites/default/files/checkout/sites/default/default.settings.php', '/var/lib/drupaltestbot/sites/default/files/checkout/sites/simpletest/996777/settings.php')

This change is really going to help people what has gone wrong. Nice work on a nearly 7 year old bug.

I agree that there is no disruption with this change and it is tested. Committed 2c0f5e1 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/dblog/src/Logger/DbLog.php b/core/modules/dblog/src/Logger/DbLog.php
index 69fbdb5..03ecbd8 100644
--- a/core/modules/dblog/src/Logger/DbLog.php
+++ b/core/modules/dblog/src/Logger/DbLog.php
@@ -45,7 +45,7 @@ class DbLog implements LoggerInterface {
   /**
    * Constructs a DbLog object.
    *
-   * @param \Drupal\Core\Database\Connection $database
+   * @param \Drupal\Core\Database\Connection $connection
    *   The database connection object.
    * @param \Drupal\Core\Logger\LogMessageParserInterface $parser
    *   The parser to use when extracting message variables.

Minor fix on commit.

Looks like this issue affects Drupal 7 too? (In fact, was originally filed for Drupal 6 :)

Rerolled patch to D7.

This is the code that was committed to 8.x http://cgit.drupalcode.org/drupal/commit/?id=2c0f5e1

@aerozeppelin it seems you rerolled a different patch.

aerozepellin's patch in #63 is for D7, @dagmar are you saying it doesn't accomplish the same thing?