Provide a storage backend for dblog module

@chx Do you mean this type of sql bound?

I'm working on this. Just want to understand if this is the right approach for this issue.

Needs test for the new method.

I have rerolled the patch for version 8.2.x

Quick review:

+++ b/core/modules/dblog/src/Form/DblogClearLogConfirmForm.php
@@ -65,7 +39,7 @@ public function getCancelUrl() {
-    $this->connection->truncate('watchdog')->execute();

+++ b/core/modules/dblog/src/Logger/DbLog.php
@@ -109,4 +109,10 @@ public function log($level, $message, array $context = array()) {
+    $this->connection->delete('watchdog')->execute();

You are removing a truncate action and adding a delete action.

Thanks everyone for working on this. But before trying to make this work, I think we should have an answer for my question on #3.

Also, I we are going to wrap all the SQL sentences of the dblog module to be able to swap them later, we should define a plan to do this. Where to store them, is the core/modules/dblog/src/Logger/DbLog.php the right place?

@dagmar: I think that you are on the right way with your approach in comment #3. Especially literal SQL string should move to "logger.dblog" service.
That service is the right location to store them because it is a backend overridable service. If you like to store the dblogs on something other then the 3 by drupal core supported databases you can override the "logger.dblog" service and it will works for your other database. Even for a NOSQL database like MongoDB.

Some examples are:

1. In the method DblogController::eventDetails(): $this->database->query('SELECT w.*, u.uid FROM {watchdog} w LEFT JOIN {users} u ON u.uid = w.uid WHERE w.wid = :id', array(':id' => $event_id))->fetchObject())
2. In the function _dblog_get_message_types(): db_query('SELECT DISTINCT(type) FROM {watchdog} ORDER BY type')->fetchAllKeyed(0, 0);

For the patch in comment #8:

+++ b/core/modules/dblog/src/Form/DblogClearLogConfirmForm.php
@@ -14,32 +14,6 @@
   /**
-   * The database connection.
-   *
-   * @var \Drupal\Core\Database\Connection
-   */
-  protected $connection;
-
-  /**
-   * Constructs a new DblogClearLogConfirmForm.
-   *
-   * @param \Drupal\Core\Database\Connection $connection
-   *   The database connection.
-   */
-  public function __construct(Connection $connection) {
-    $this->connection = $connection;
-  }
-
-  /**
-   * {@inheritdoc}
-   */
-  public static function create(ContainerInterface $container) {
-    return new static(
-      $container->get('database')
-    );
-  }

I think that removing the database connection is a good idea. Instead we should add the "logger.dblog" service in the same way.

I'm going to work on this. Stills needs work but I would like to see how the tests are failing.

Ok, this is now ready for a deeper review.

I think I should create a new interface to document all this new methods. Any suggestion for its name?

Patch look good. I do have some remarks:

1. +++ b/core/modules/dblog/src/Controller/DbLogController.php
   @@ -151,28 +138,14 @@ public function overview() {
   +    $result = \Drupal::service('logger.dblog')->getCollection(
   

   Can you add the logger.dblog service with the __construct method.

2. +++ b/core/modules/dblog/src/Controller/DbLogController.php
   @@ -237,13 +210,19 @@ public function overview() {
          $username = array(
            '#theme' => 'username',
            '#account' => $dblog->uid ? $this->userStorage->load($dblog->uid) : User::getAnonymousUser(),
          );
   

   Should this piece of code not be removed? We are adding:

   +++ b/core/modules/dblog/src/Controller/DbLogController.php
   @@ -237,13 +210,19 @@ public function overview() {
   +      $username = array(
   +        '#theme' => 'username',
   +        '#account' => $this->userStorage->load($dblog->uid) ?: User::getAnonymousUser(),
   +      );
   

3. +++ b/core/modules/dblog/src/Logger/DbLog.php
   @@ -109,4 +109,146 @@ public function log($level, $message, array $context = array()) {
   +  /**
   +   * Get a collection of logs.
   +   */
   +  public function getCollection($filter, $sorting, $limit) {
   ...
   +  /**
   +   * Deletes all the logs.
   +   */
   +  public function deleteAll() {
   

   The parameters and the return value of these methods are not documented.

4. +++ b/core/modules/dblog/src/Logger/DbLog.php
   @@ -109,4 +109,146 @@ public function log($level, $message, array $context = array()) {
   +  function types() {
   

   I think that it is better to rename this method to getTypes(). And it will be a "public" method.

5. +++ b/core/modules/dblog/src/Logger/DbLog.php
   @@ -109,4 +109,146 @@ public function log($level, $message, array $context = array()) {
   +
   

   Nitpick: This line can be removed.

6. +++ b/core/modules/dblog/src/Logger/DbLog.php
   @@ -109,4 +109,146 @@ public function log($level, $message, array $context = array()) {
   +
   

   Nitpick: This line can be removed.

7. I think that it will be good to create an interface for it. What do you think of DbLogInterface. It should extend Psr\Log\LoggerInterface.

Thanks @daffie.

Items 1, 2, 3, 4 and 7 should be fixed now. 5 and 6 probably you made some mistake with dreditor.

Looking at the new interface definition

I think we should lock down some of the input parameters to be arrays.

Plus I ran the new interface through phpcs and picked out minor nits.

@dagmar, sorry

I take the line .... if I broke it, I fix it..

If you don't mind I want to take ownership for a short time until tests pass.

I am working on this now...I hope that is OK

DbLogController::buildFilterQuery was returning NULL early when the session was not primed

I think it makes more sense to return an empty array -- so we have a consistent array type when it feeds it output into DbLog::getCollection()

Thanks @martin107 I'm going to start writing some Kernel tests this week.

This patch removes the last occurrence of $connection from outside of the service.

Here are the initial kernel tests.

Renamed core/modules/dblog/tests/src/Kernel/DbLog.php to core/modules/dblog/tests/src/Kernel/DbLogTest.php

Sorry for my late review. The patch looks great, some minor points left:

1. +++ b/core/modules/dblog/tests/src/Kernel/DbLogTest.php
   @@ -0,0 +1,158 @@
   + *
   + * Tests for the DbLog class.
   

   These lines are not necessary. The @coversDefaultClass tells you what this class will test.

2. +++ b/core/modules/dblog/src/Logger/DbLogInterface.php
   @@ -0,0 +1,70 @@
   +   * @return array
   +   *   List of uniquely defined database log message types.
   

   Can we improve this. Key and the value are the log message.

3. +++ b/core/modules/dblog/src/Logger/DbLogInterface.php
   @@ -0,0 +1,70 @@
   +   *   An array of log entries.
   

   Can you improve this description. Key is the message and the value is the number of appearances.

4. +++ b/core/modules/dblog/src/Controller/DbLogController.php
   @@ -237,12 +222,18 @@ public function overview() {
          $username = array(
            '#theme' => 'username',
   -        '#account' => $dblog->uid ? $this->userStorage->load($dblog->uid) : User::getAnonymousUser(),
   +        '#account' => $this->userStorage->load($dblog->uid) ?: User::getAnonymousUser(),
   ...
   +      $username = array(
   +        '#theme' => 'username',
   +        '#account' => $this->userStorage->load($dblog->uid) ?: User::getAnonymousUser(),
   

   Why do we do this twice?

5. +++ b/core/modules/dblog/src/Logger/DbLogInterface.php
   @@ -0,0 +1,70 @@
   +   * @return Object
   

   Not completely happy about this description, but I do not know how to improve it.

6. +++ b/core/modules/dblog/tests/src/Kernel/DbLogTest.php
   @@ -0,0 +1,158 @@
   +
   +
   

   Nitpick: Double empty line.

Thanks!

5.
+++ b/core/modules/dblog/src/Logger/DbLogInterface.php
@@ -0,0 +1,70 @@
+ * @return Object
Not completely happy about this description, but I do not know how to improve it.

This is the only missing point of this patch.

I spent some time reviewing my own patch. I added more tests to cover some edge cases.

Also I worked a bit on the docs. All the references to watchdog in the interface were removed.

I think this is quite complete to be merged. What do you think @daffie?

The patch is almost RTBC for me.

I have thought about how to document the return object in DbLogInterface::get() any better and I have no idea. It is just a standard php class.

I have just 1 remark left after your last patch:

+++ b/core/modules/dblog/src/Logger/DbLogInterface.php
@@ -0,0 +1,72 @@
+   * @param array $filter
+   *   An array of filters to filter the collection.
...
+  public function getCollection(array $filter, array $sort, $limit);

+++ b/core/modules/dblog/tests/src/Kernel/DbLogTest.php
@@ -0,0 +1,172 @@
+    $filter = [
+      'where' => 'w.severity = ?',
+      'args' => [RfcLogLevel::WARNING]
+    ];

The filter parameter is an array with two key values. Can you document them.

Here is the patch, not sure about the description though. Please Suggest me the right description, if there is a problem with this.

The patch looks now good enough for me.

I have 1 remark left and that can be fixed on commit:

+++ b/core/modules/dblog/src/Logger/DbLogInterface.php
@@ -0,0 +1,74 @@
+   * @param array $filter
+   *   An array of filters to filter the collection.
+   * - where: The filter condition.
+   * - args: A list of arguments.

Can we change it to "An array with the filter data to filter the collection. The following array keys can be set:"

Thank you @dagmar, @martin107, @Yogesh_Pawar and @himanshu-dixit for all your work.

Here is the new patch, I am not sure weather i should split the suggested description in two line to avoid 80 cols violations on it. @daffie Please review this patch.

UPDATE: Oops, i add '.patch' extension to interdiff and now that would fail. :(

The patch looks now good enough for me.

As expected, interdiff.patch wouldn't be able to apply (Sorry i shouldn't have saved it as patch). Returning RTBC Status

+++ b/core/modules/dblog/src/Controller/DbLogController.php
@@ -237,12 +222,14 @@ public function overview() {
-        '#account' => $dblog->uid ? $this->userStorage->load($dblog->uid) : User::getAnonymousUser(),
+        '#account' => $this->userStorage->load($dblog->uid) ?: User::getAnonymousUser(),

Why this change? It looks unnecessary. And could cause additional queries for no reason.

Here is the new patch. @alexpott Please review this patch.

Oops, the interdiff was big because i pull origin 8.4.x for one of the new branch. Here is the correct interdiff

NR for #41.

+++ b/core/modules/dblog/dblog.module
@@ -87,8 +71,7 @@ function dblog_cron() {
 function _dblog_get_message_types() {

Let's deprecate this. Ideally the replacement would protected and this just not exposed. Hmmm... although:

    'filter' => array(
      'id' => 'in_operator',
      'options callback' => '_dblog_get_message_types',
    ),

From dblog_views_data() - so the underscore is misleading.

Also I think we should consider moving dblog_filters() to the expanded service and deprecate it. This is because it is involved in the SQL syntax for building the query.

The other thing I wonder if should we decouple storage completely from logging - ie. inject a DbLogStorage service into the logger.dblog service. This is for two reasons, one we'd have less API change and more API addition and secondly, it'd be possible to make the watchdog table create lazy so sites which swap out service don't end up with a watchdog table and can call it what they like and structure it right. Lastly,

+++ b/core/modules/dblog/src/Logger/DbLog.php
@@ -109,4 +108,125 @@ public function log($level, $message, array $context = array()) {
+  public function get($id) {
...
+  public function getCollection(array $filter = [], array $sort = [], $limit = 50) {
...
+  public function topLogMessages($type) {

If we really want this to be swappable just returning \stdClass with public properties doesn't offer much of an interface. We could implement a value object with getters and a magic method to get the protected properties when someone does $result->wid for BC.

Thanks alexpott for the review. I agree with everything you said. This is the reason #2847428: [Meta] Modernize dblog module was created.

I would like to know what is the minimal tasks we should include on this issue to commit it and iterate over the other issues.

My concern is if we try to change everything you mentioned into a single patch we will end with a entirely new dblog module in a single file to review.

Back to NR to get an answer on that. Also credited daffie he reviewed all my patches.

@dagmar I think this issue should be concerned with inject a storage service into the logger. And doing the minimal to make that work. It should also deprecate anything that should be deprecated as a result of this. It does not have to deal with auto schema generation. But on commit all the SQL concerns should be part of this service. The current state of the issue is not a good halfway house as it introduces a big API change when what we really want is #44.

Thanks @alexpott.

I'll be working on this during next week.

Here is the first try of the new approach proposed by @alexpott.

Overall I think the proposed solution makes much more sense since there is no big changes to the Logger class but an addition.

I have some questions to discuss here:

1. I'm introducing a general Log class to standardize logs in drupal. I tried to make the interface as simpler as possible following the same conventions than Psr3 propose (level, message and context). Comments regarding this addition are welcome.
2. Should I include setters on this class?
3. I moved the formatMessage method from the DbLogController to the DbLog class. This makes sense to me, is this considered a BC break?
4. If this new class is ok for everyone, we have to discuss what to do with the REST endpoint. Before all the columns of the watchdog table were fields of a StdClass, now some of them are grouped into the context attribute and severity was renamed to level

There is some pending work to do regarding views integration. I'm posting this to see what the testbot thinks.

Fixed all coding standards highlighted by the testbot in#48. This parch introduces a custom views filter for dblog types and therefore _dblog_get_message_types can be deprecated.

Also I'm providing a possible (but dirty) approach for BC regarding rest integration.

@dagmar - this change should try to limit its scope as much as possible and just be about the storage.

+++ b/core/lib/Drupal/Core/Logger/LogInterface.php
@@ -0,0 +1,30 @@
+interface LogInterface {

+++ b/core/modules/dblog/src/DbLog.php
@@ -0,0 +1,136 @@
+class DbLog implements LogInterface {

Why is this required to do that? Also calling the new class DbLog is unfortunate because it clashes with Logger\DbLog just to make things more confusing.

And yep #48.3 feels an unnecessary db break.

Re #48.1 I don't think setters are appropriate - log messages are immutable!

@alexpott: Thank you for your help!

This patch includes the minimal changes (most of them additions to the existent API) to be able to swap the storage.

Good news, there are is no BC break in this patch.

@dagmar - this change should try to limit its scope as much as possible and just be about the storage.

Agree. Sorry about that. I reverted all the unnecessary changes and the new interface was removed.

Why is this required to do that? Also calling the new class DbLog is unfortunate because it clashes with Logger\DbLog just to make things more confusing.

Fixed.

And yep #48.3 feels an unnecessary db break.

Fixed.

Re #48.1 I don't think setters are appropriate - log messages are immutable!

Makes sense :)

Thanks for all your help @alexpott

A quick first review. Maybe there are some stupid questions. Hope you do not mind.

1. +++ b/core/modules/dblog/dblog.services.yml
   @@ -1,7 +1,12 @@
   -    arguments: ['@database', '@logger.log_message_parser']
   +    arguments: ['@storage.dblog', '@logger.log_message_parser']
   

   I think this is a BC break.

2. +++ b/core/modules/dblog/src/Logger/DbLog.php
   @@ -19,16 +17,11 @@ class DbLog implements LoggerInterface {
   -  const DEDICATED_DBLOG_CONNECTION_TARGET = 'dedicated_dblog';
   

   You cannot remove a constant. This will result in a BC break.

3. +++ b/core/modules/dblog/src/Log.php
   @@ -0,0 +1,212 @@
   +  /**
   +   * Unique log event ID.
   +   * @var int
   +   */
   +  protected $wid;
   

   The API documentation and comment standards states that there must have a blank line between "Unique log event ID." and "@var int". The same for the rest of this class.

4. +++ b/core/modules/dblog/src/Controller/DbLogController.php
   @@ -56,35 +49,42 @@ class DbLogController extends ControllerBase {
   -  public function __construct(Connection $database, ModuleHandlerInterface $module_handler, DateFormatterInterface $date_formatter, FormBuilderInterface $form_builder) {
   ...
   +  public function __construct(ModuleHandlerInterface $module_handler, DateFormatterInterface $date_formatter, FormBuilderInterface $form_builder, DbLogStorageInterface $dblog_storage) {
   

   Not sure if this is a BC break.

5. +++ b/core/modules/dblog/src/Log.php
   @@ -0,0 +1,212 @@
   +  /**
   +   * Creates a Log entry.
   +   *
   +   * @param array $data
   +   *   An array that contains the info of the log. Where keys are the colums of
   +   *   the watchdog table.
   +   */
   +  public function __construct(array $data) {
   +    $internal_properties = array_keys(get_object_vars($this));
   +
   +    foreach ($data as $key => $value) {
   +      if (is_string($key) && in_array($key, $internal_properties)) {
   +        $this->$key = $value;
   +      }
   +    }
   +  }
   

   Should this method not be the first method in this class?

6. +++ b/core/modules/dblog/src/Storage/DbLogStorage.php
   @@ -0,0 +1,238 @@
   +  const TABLE = 'watchdog';
   ...
   +  const DEDICATED_DBLOG_CONNECTION_TARGET = 'dedicated_dblog';
   

   Shouldn't these two constants be added to the interface?

7. +++ b/core/modules/dblog/src/Storage/DbLogStorage.php
   @@ -0,0 +1,238 @@
   +  public function mostFrequentLogs($type) {
   +
   

   This empty line is not needed.

Not sure on how to address backward compatibility on #54.1 , .4 and .6

Thanks @daffie and @tameeshb.

The BC compatibility is still being discussed here: #2116841: [policy] Decide what the Drupal Public API should be and how to manage it.

From what I understand (please correct me if I'm wrong), changes in services and constructors are transparent for developers that load them using \Drupal:: service().

This is the relevant part from the mentioned issue.

Constructors for service objects, plugins, controller:

The constructor for a service object (one in the container), plugin, or controller is considered internal unless otherwise marked, and may change in a minor release. These are objects where developers are not expected to call the constructor directly in the fist place. Constructors for value objects where a developer will be calling the constructor directly are excluded from this statement.

You cannot remove a constant. This will result in a BC break.

Ok, nice catch. But we should include a @deprecate mention and point devs to use the defined in LogStorage.

@dagmar: For #54.1 and #54.4, I agree with you that those are not BC breaks.

Two nitpicks:

1. +++ b/core/modules/dblog/src/Log.php
   @@ -0,0 +1,238 @@
   +   * @param string $key
   +   * @return mixed
   

   There needs to be a blank line inserted between these lines.

2. +++ b/core/modules/dblog/src/Logger/DbLog.php
   @@ -22,13 +20,13 @@ class DbLog implements LoggerInterface {
   -
   

   These spaces are not needed.

@gaurav.kapoor thanks, but it seems you missed some files on your patch.

#54.6 Shouldn't these two constants be added to the interface?

In my opinion, no. If you use a NoSql database system to replace the storage, you could not need a dedicated connection. Also the concept of 'table' could be different in another system.

Added the @deprecated comment the DEDICATED_DBLOG_CONNECTION_TARGET constant.

@daffie fixed your comments from #57. Thanks.

Minor Fixes.

I had fixed that and was uploading new patch , got an error that someone else changed Show/Hide files. Anyway it looks good now. We should add more tests to it.

I appreciate the help @gaurav.kapoor. But please make sure you include an interdiff https://www.drupal.org/documentation/git/interdiff when posting new patches.

Thanks!

1. +++ b/core/modules/dblog/dblog.module
   @@ -61,34 +61,20 @@ function dblog_cron() {
    /**
     * Gathers a list of uniquely defined database log message types.
     *
   + * @deprecated in Drupal 8.4.x and will be removed before 9.0.0. Use
   + *   \Drupal::service('storage.dblog')->getTypes() instead.
   + *
     * @return array
     *   List of uniquely defined database log message types.
     */
    function _dblog_get_message_types() {
   -  return db_query('SELECT DISTINCT(type) FROM {watchdog} ORDER BY type')
   -    ->fetchAllKeyed(0, 0);
   +  return \Drupal::service('storage.dblog')->getTypes();
    }
   

   Needs to use the new deprecation policy see https://www.drupal.org/node/2856615

2. +++ b/core/modules/dblog/src/Log.php
   @@ -0,0 +1,212 @@
   +
   +class Log {
   
   +++ b/core/modules/dblog/src/Logger/DbLog.php
   @@ -61,52 +54,28 @@ public function log($level, $message, array $context = array()) {
   +    ]);
   +
   

   Needs a class docblock and this thing should probably be called something like LogEntry or LogMessage.

Thanks @alexpott!. Added a change record here: https://www.drupal.org/node/2857426

Also updated the issue summary to reflect the API additions.

Sorry but we need an upgrade path :(

The change of the type filter will break all the views that use this filter. And the upgrade path for the view integration is out of the scope of this issue.

I'm already working on an issue to upgrade all the dblog views handlers so the plan is:

1. Move the code related to the views integration here #2851293: dblog is using the wrong views field, filter and relationships definitions.
2. Write the upgrade path and the test using the _dblog_get_message_types function.
3. Commit that patch and then just change one line of code on the filter in this patch.

While working on #2852990: Deprecate dblog_filters I was thinking in this comment from #33.

The filter parameter is an array with two key values. Can you document them.

I think we have the opportunity to change that. The storage service should receive the items the keys and values to filter and build the query inside. Currently this is being done by a controller. See: DbLogController::buildFilterQuery().

So probably the $filter passed into loadMultiple could look like this:

$filter = [
  'type' => ['access denied', 'page not found'],
  'severity' => [2, 4],
];

And then the storage service can defined the best query to achieve that.

@daffie what do you think?

Good news. #2851293: dblog is using the wrong views field, filter and relationships definitions is ready for review.

I updated the issue summary to reflect what is missing here. I will try to review this patch and provide feedback as dblog maintainer. Contributors are welcome.

Re-rolled.

Thanks!

1. +++ b/core/modules/dblog/src/LogEntry.php
   @@ -0,0 +1,241 @@
   +class LogEntry {
   

   Ideally we should have some Unit tests for this class.

2. +++ b/core/modules/dblog/src/Storage/DbLogStorage.php
   @@ -0,0 +1,236 @@
   +    if (!empty($filter['where'])) {
   +      $query->where($filter['where'], $filter['args']);
   +    }
   +
   

   This needs to be refactored. Something like $query->condition(field, value, operator);

3. +++ b/core/modules/dblog/src/Storage/DbLogStorage.php
   @@ -0,0 +1,236 @@
   +      ->fields('w', array('message', 'variables'))
   

   There is an array() here.

4. +++ b/core/modules/dblog/src/Storage/DbLogStorageInterface.php
   @@ -0,0 +1,84 @@
   +   *   An array with the filter data to filter the collection. The following
   +   *   array keys can be set:
   +   *   - where: The filter condition.
   +   *   - args: A list of arguments.
   

   Needs to be refactored. See issue summary.

5. +++ b/core/modules/dblog/tests/src/Kernel/DbLogStorageTest.php
   @@ -0,0 +1,159 @@
   +    $this->installSchema('dblog', array('watchdog'));
   

   change array() to []

6. +++ b/core/modules/dblog/tests/src/Kernel/DbLogStorageTest.php
   @@ -0,0 +1,159 @@
   +    $filter = [
   +      'where' => 'w.severity = ?',
   +      'args' => [RfcLogLevel::WARNING]
   +    ];
   

   This needs to be refactored.

Two types of changes nothing major

1) where we have string such as

'Drupal\Core\Database\Query\PagerSelectExtender'

PagerSelectExtender::class is the same FQDN string BUT it is more semantically meaningful.
If the next commit changes the class name to say PageModifierExtender then PHP will automatically flag these strings up, if the change is not made consistently throughout the project.

2) I passed the new files through phpcs and fixed trivial nits.

PS sorry comment clash with #74

Accidently I have fixed 6,5,3

more tomorrow night.

Amazing thanks @martin107.

So to recap there are 3 things missing here.

First:

  /**
   * Builds a query for database log administration filters based on session.
   *
   * @return array
   *   An associative array with keys 'where' and 'args'.
   */
  protected function buildFilterQuery() {
    if (empty($_SESSION['dblog_overview_filter'])) {
      return;
    }

    $this->moduleHandler->loadInclude('dblog', 'admin.inc');

    $filters = dblog_filters();

    // Build query.
    $where = $args = [];
    foreach ($_SESSION['dblog_overview_filter'] as $key => $filter) {
      $filter_where = [];
      foreach ($filter as $value) {
        $filter_where[] = $filters[$key]['where'];
        $args[] = $value;
      }
      if (!empty($filter_where)) {
        $where[] = '(' . implode(' OR ', $filter_where) . ')';
      }
    }
    $where = !empty($where) ? implode(' AND ', $where) : '';

    return [
      'where' => $where,
      'args' => $args,
    ];
  }

This logic has to be abstracted from the controller. No SQL queries should be returned here. Instead we could just use something like the parameters accepted by QueryInterface::condition:

[$field, $value, $operator]

Example:

return [
   ['severity', [1, 4], 'IN'],
   ['type', ['access denied', 'user'], 'IN']
];

Secondly, we need refactor DblogStorage::loadMultiple() to accept that change and write the test coverage.

And third. We need unit test for the new LogEntry class.

I will do the refactoring needed here (#78)

but first I want to remove a side issue.

$_SESSION['dblog_overview_filter'] is a global variable and that really sticks in my craw.

So do you mind if I engage in some grumpy driven development and take it out before proceeding.

dagmar has convinced me not to worry about the global variable.

https://www.drupal.org/node/2877985#comment-12083604

@martin107 if unasigning this ticket to allow others to contribute. The issue summary describes in the Remaining Tasks section what is missing here.

This patch includes the following changes:

• Adds unit test for LogEntry.
• Make the DblogStorageInterface smaller (only includes CRUD related methods: save, load, loadMultiple, deleteAll, deleteOldItems and an extra info method getTypes).
• Add test coverage and fixes the use of sorting in DblogStorage::loadMultiple()
• Deprecates dblog_filter()

By making the interface less strict now it wont be possible to swap the backend and get the UI working. But I think this is a good thing, because forcing the storage to implement every single method required by the controller makes the storage really complex.

For the record, this issue is part of the set of abstraction changes we (mostly chx TBH) uncovered while working on the 8.x-1.x branch of the MongoDB module, which was a SQL-less distribution of Drupal. Having to support actually switching the database for something entirely different uncovered a number of cases like this one.

In relation with #1506262: Change dblog_watchdog to use Drupal Queue instead of DB insert., this makes me remember an important assumption currently done by dblog: the notion that queries are synchronous. When going with an asynchronous storage (say a queue), some integration tests will necessarily fail if they check that the data arrived "at the other end".

Re-rolled. Didn't apply due #2901739: Fix 'Squiz.Arrays.ArrayDeclaration' coding standard

It would be interesting to see if you can map this abstraction to the existing MongoDB watchdog (i.e. logger) in the 8.x-2.x branch of the MongoDB module and/or the Redis watchdog in the Redis module, or if the abstraction is not sufficient to support this overriding.

Way back in 2014, the ability to actually support a NoSQL implementation was why so many storage patches of this type were created while @chx was still working on the 8.x-1.x version of the MongoDB, showing to what extent our existing DB abstractions were still leaking.

Rerolled after

• #2935256: Remove all usages of drupal_get_message and drupal_set_message in modules
• #2849669: Fix \Drupal\Component\Utility\Unicode() because of the Symfony mbstring polyfill
• #2958429: Properly deprecate SafeMarkup::format()
• #2901739: Fix 'Squiz.Arrays.ArrayDeclaration' coding standard

Rerolled after

• #2935256: Remove all usages of drupal_get_message and drupal_set_message in modules
• #2849669: Fix \Drupal\Component\Utility\Unicode() because of the Symfony mbstring polyfill
• #2958429: Properly deprecate SafeMarkup::format()
• #2901739: Fix 'Squiz.Arrays.ArrayDeclaration' coding standard

I've been readying the mongodb 8.2 branch for a release. Can we make sure the changes here and there will be compatible so mongodb_watchdog can still work cleanly when this issue is eventually merged.

In the meantime, AIUI the "backend_overridable" tag remaining on the logger.dblog service definition is not correct: this is a tag meant to be used by services injecting the database, and with this patch, that service no longer injects the database, so it no longer needs this tag.

For referench about backend_overridable : https://www.drupal.org/node/2306083

After reading part of the code, this is definitely interesting, but the use of a concrete LogEntry class is an issue, as it will be difficult to reconcile the DbLog usage with other logging mechanisms. It would probably be better to depend on factories and interfaces instead of that single concrete class. Specifically, having DbLog and DbLogStorage both instantiate the hard-coded LogEntry instead of relying on a factory service makes them harder to reuse and interface with.

Here is a comparison of the classes involved for the actual message in DbLog (LogEntry) vs MongoDB (Event/EventTemplate). The main difference is how MongoDB splits the constant part (EventTemplate) from the variant part (Event) to provide user-level aggregation instead of a plain sequential list.

• LogEntry:
  • `__get`/`__isset` provide property access instead of getters.
• EventTemplate:
  • ET::keys(): provides labels and callbacks per key
  • ET::bsonUnserialize() ` MongoDB\BSON\Unserializable` helper.
• Event:
  • E::bsonUnserialize() ` MongoDB\BSON\Unserializable` helper.

A new attempt to address this issue is now here: #2401463: Make dblog entities

A full DblogStorage implementation with tests is now available here #2401463: Make dblog entities