dblog is using the wrong views field, filter and relationships definitions

@dagmar since the relationship isn't changed in the post_update hook there is no need to test the change in the post_update test. A normal (non-update) test that checks that relationship might be nice.

bit of quick nitpicking:

1. +++ b/core/modules/dblog/dblog.post_update.php
   @@ -0,0 +1,77 @@
   +/**
   + * @file
   + * Post update functions for the Database Logging module.
   + */
   

   I don't think we do @file anymore?

2. +++ b/core/modules/dblog/dblog.post_update.php
   @@ -0,0 +1,77 @@
   + * Change the table relationship of user for watchdog. Use standard plugin for
   

   The relationship isn't changed in the post update hook.

3. +++ b/core/modules/dblog/dblog.post_update.php
   @@ -0,0 +1,77 @@
   +          // We are only interested in wid and uid fields from the watchdog
   +          // table that still use the numeric id
   

   missing period at the end

4. +++ b/core/modules/dblog/dblog.views.inc
   @@ -39,9 +47,8 @@ function dblog_views_data() {
   -    'help' => t('The user ID of the user on which the log entry was written..'),
   

   do we want to remove this?

It seems we cannot test the view with the previous values defined in the hook_views_data. Some advice from the views maintainers here?

To be honest I'm personally not convinced by testing things before running update.php. Accessing stuff can cause side effects, like maybe some caching of schema in this issue.

I think the removal of the VDC tag was an xpost. Tagging for subsystem maintainer review also to make sure @dawehner's comments in #8 are taken into consideration before this might be committed.

Accessing stuff can cause side effects, like maybe some caching of schema in this issue.

Uhh if running update.php doesn't clear those caches, isn't that something we would want to detect? Cause that sounds like trouble to me :)

Manually tested this and the changes are good and the update works nicely.

1. +++ b/core/modules/dblog/dblog.views.inc
   @@ -39,9 +47,9 @@ function dblog_views_data() {
   +    'help' => t('The user ID of the user on which the log entry was written.'),
   

   Unrelated change

2. +++ b/core/modules/dblog/src/Plugin/views/filter/DblogTypes.php
   @@ -0,0 +1,24 @@
   +      $this->valueOptions = _dblog_get_message_types();
   

   This seems very strange to me. This is great for the exposed filter, but for a non-exposed filter this makes no sense. This means when I'm building the View, the type of message I want to filter on, needs to be present in the database at that time. But it seems that was how this previously worked too, so this is not the place to change this behaviour probably.

3. +++ b/core/modules/dblog/src/Tests/Update/DblogUpgrade2851293.php
   @@ -0,0 +1,47 @@
   +class DblogUpgrade2851293 extends UpdatePathTestBase {
   

   Class name can be a bit more descriptive and needs to end in Test (I think)

4. +++ b/core/modules/dblog/src/Tests/Update/DblogUpgrade2851293.php
   @@ -0,0 +1,47 @@
   +    // type should use the dblog_types plugin now.
   

   Start with a capital letter or quote type.

5. +++ b/core/modules/dblog/tests/src/Kernel/Views/ViewsIntegrationTest.php
   @@ -40,19 +41,137 @@ protected function setUp($import_test_views = TRUE) {
   +    $entries = $this->createRandomLogEntries();
   ...
   +    $this->createRandomLogEntries();
   ...
   +  protected function createRandomLogEntries() {
   

   Why random? why not use fixed test parameters?

6. +++ b/core/modules/dblog/tests/src/Kernel/Views/ViewsIntegrationTest.php
   @@ -40,19 +41,137 @@ protected function setUp($import_test_views = TRUE) {
   +    $this->container->get('router.builder')->rebuild();
   

   not needed

7. +++ b/core/modules/dblog/tests/src/Kernel/Views/ViewsIntegrationTest.php
   @@ -40,19 +41,137 @@ protected function setUp($import_test_views = TRUE) {
   +    $this->container->get('router.builder')->rebuild();
   

   not needed

1. Yeah I think it's fine sneaking it in here.

2. For the non-exposed filter a string filter would make more sense to me, but since we can't vary by exposed/non-exposed and this was how it was working before, so yeah lets stick with this.

5. Yeah that follow up sounds like the way to go, so this looks good for now.

6/7:

+++ b/core/modules/dblog/tests/src/Kernel/Views/ViewsIntegrationTest.php
@@ -18,41 +18,162 @@
+    $this->container->get('router.builder')->rebuild();
...
+    $this->container->get('router.builder')->rebuild();

You got one that I missed, and missed two that I pointed to :)

So just 6/7 and then this looks ready to me.

+++ b/core/modules/dblog/tests/src/Kernel/Views/ViewsIntegrationTest.php
@@ -76,41 +195,28 @@ public function testIntegration() {
+    // Setup a watchdog entry with severity WARNING
...
+    // Setup a watchdog entry with a different module

Comments should (noramlly) begin with a capital letter and end with a full stop / period .

Fixed and attached new patch.

This looks ready to me. Nice work!

Coder is not happy with this one:

core/modules/dblog/dblog.post_update.php
line 1 Missing file doc comment
core/modules/dblog/tests/src/Kernel/Views/ViewsIntegrationTest.php
30 Line indented incorrectly; expected 2 spaces, found 3

Also:

+++ b/core/modules/dblog/dblog.post_update.php
@@ -0,0 +1,72 @@
+ * Use standard plugin for wid and uid fields for watchdog table, and the
+ * dblog_types plugin for type filter.

This should be shortened so it fits in one line.

Thank you, perfect!

Some coding improvements. Should we wait for @dawehner here so that we can remove the subsystem review tag?

1. +++ b/core/modules/dblog/dblog.post_update.php
   @@ -0,0 +1,76 @@
   +            $save = TRUE;
   ...
   +            $save = TRUE;
   

   Why not $save |= TRUE?

2. +++ b/core/modules/dblog/dblog.post_update.php
   @@ -0,0 +1,76 @@
   +            // Delete all the attributes related to numeric fields.
   +            foreach ($numeric_attributes_to_delete as $key) {
   +              if (isset($new_value[$key])) {
   +                unset($new_value[$key]);
   +              }
   

   We don't need if here. Just unset($new_value[$key]); is fine.
   This can all be done in one line. unset($arr[key1], $arr[key2], ...);

3. +++ b/core/modules/dblog/src/Plugin/views/filter/DblogTypes.php
   @@ -0,0 +1,24 @@
   +      $this->valueOptions = _dblog_get_message_types();
   

   Let's inject the query service and execute the query here.

Well @Lendude is also a subsystem maintainer but yeah only a subsystem maintainer should remove that tag. Given that @Lendude rtbc-d the patch I assume he is happy with it but I'd still rather wait for confirmation.

1. Some tiny nits that would be nice if they were addressed. Not going to needs work for this. Nits patch attached

2. +++ b/core/modules/dblog/tests/src/Kernel/Views/ViewsIntegrationTest.php
   @@ -18,41 +18,160 @@
   +        'plugin_id' => 'in_operator',
   +      ]
   +    ];
   ...
   +        'plugin_id' => 'dblog_types',
   +      ]
   +    ];
   

   Strictly there should be a comma after the ]

3. +++ b/core/modules/dblog/tests/src/Kernel/Views/ViewsIntegrationTest.php
   @@ -18,41 +18,160 @@
   +   * Create a set of log entries.
   +   * @return array
   

   New line in between the first line and the @return.

4. +++ b/core/modules/dblog/tests/src/Kernel/Views/ViewsIntegrationTest.php
   @@ -76,41 +195,28 @@ public function testIntegration() {
   +      'type' => 'my-module'
   

   Need comma at the end.

Discussed with @catch - we think we need to fix this using hook_ENTITY_TYPE_presave() so that default views supplied by contrib and custom modules can be updated too. This means the hook_update_N() needs to determine which views need to be saved again and save them.

I disagree with this direction hook_ENTITY_TYPE_presavewill be called whenever a view is saved which is not ideal imo. The view integration is introduced in #1874534: Introduce a dblog / watchdog views integration. I doubt that contrib is using this but that's beside the point. We never cared about the default views but we do update the existing views which this patch was already fixing. Updating the config object in preSave means we are creating a mismatch between the default and saved config which is not right.
Consider the other side of the picture for a moment not fixing the default views would show the error after importing and views should have to be updated and saved and that's what we actually want so that config can be updated properly.

If we still want to go down preSave road then every time we change the existing views data we have to write the preSave for the respective module which is not ideal. If BC break is the issue then we should consider keeping the original mapping as is and we should consider the new mapping so that contrib can catch up then we can even nuke the update hook altogether. Thoughts?

+++ b/core/modules/dblog/dblog.views.inc
@@ -20,11 +20,19 @@ function dblog_views_data() {
-      'id' => 'numeric',
+      'id' => 'standard',

@@ -39,9 +47,9 @@ function dblog_views_data() {
-      'id' => 'numeric',
+      'id' => 'standard',

@@ -52,7 +60,7 @@ function dblog_views_data() {
-      'base' => 'users',
+      'base' => 'users_field_data',

These changes qualify this patch as a bug.

  $data['watchdog']['uid'] = [
    'title' => t('UID'),
    'help' => t('The user ID of the user on which the log entry was written..'),
    'field' => [
      'id' => 'numeric',
    ],
    'filter' => [
      'id' => 'numeric',
    ],
    'argument' => [
      'id' => 'numeric',
    ],
    'relationship' => [
      'title' => t('User'),
      'help' => t('The user on which the log entry as written.'),
      'base' => 'users',
      'base field' => 'uid',
      'id' => 'standard',
    ],
  ];

We have a relationship defined with users table. Logically speaking it is possible to create a reverse relationship and use the watchdog data in any view so I don't think checking watchdog as a base table is the right thing to do here. Do we care about this or not?

Let's update the title as well.

+++ b/core/modules/dblog/dblog.views.inc
@@ -20,11 +20,19 @@ function dblog_views_data() {
+  $data['watchdog']['table']['join'] = [
+    // Link ourselves to the {users_field_data} table.
+    'users_field_data' => [
+      'field' => 'uid',
+      'left_field' => 'uid',
+    ],
+  ];

@@ -52,7 +60,7 @@ function dblog_views_data() {
     'relationship' => [
       'title' => t('User'),
       'help' => t('The user on which the log entry as written.'),
-      'base' => 'users',
+      'base' => 'users_field_data',
       'base field' => 'uid',
       'id' => 'standard',

If we are doing former then we don't need latter. Every time users_field_data table would be available in views the former would show the watchdog fields as well. If we remove latter then it is a BC break even it is not changing the views field plugin id but we are changing the field existing relationship and we need to update those fields as well in post-update hook.

@jibran we've used preSave() before to fix default views - and we do care about default views - #2248983: Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table

It's sad that no one from VDC has reviewed that patch or +1ed the approach :(

@jibran, @dawehner and I went into that approach here #2784233: Allow multiple vocabularies in the taxonomy filter and I 'ranted' about it some more here #2810097: Allow views to provide the canonical entity URL of all entities, not just nodes..

Summary: I'm not happy with the approach, but I couldn't find anything better (tried some things in #2784233: Allow multiple vocabularies in the taxonomy filter). We need an post_module_install event that fires after every module install or something.

+++ b/core/modules/dblog/dblog.module
@@ -115,3 +116,67 @@ function dblog_form_system_logging_settings_alter(&$form, FormStateInterface $fo
+
+          // Delete all the attributes related to numeric fields.
+          foreach ($numeric_attributes_to_delete as $key) {
+            if (isset($new_value[$key])) {
+              unset($new_value[$key]);
+            }
+          }

Ideally we'd check if these need deleting before unsetting them, then @trigger_error('...', E_USER_DEPRECATED); when they're found. That way at least once we have phpunit integration with deprecations we'll be able to inform contrib modules easier.

https://www.drupal.org/core/deprecation

Oh I missed that code is just moving around rather than related to the upgrade path.

The deprecation notice could happen here.

+++ b/core/modules/dblog/dblog.module
@@ -115,3 +116,72 @@ function dblog_form_system_logging_settings_alter(&$form, FormStateInterface $fo
+
+    // Iterate all filters looking for type filters to update.
+    if (isset($display['display_options']['filters'])) {
+      foreach ($display['display_options']['filters'] as $filter_name => $filter) {
+        if (isset($filter['table']) &&
+            $filter['table'] === 'watchdog' &&
+            $filter['plugin_id'] == 'in_operator' &&
+            $filter['field'] == 'type') {
+
+          $filter['plugin_id'] = 'dblog_types';
+          $view->set("display.$display_name.display_options.filters.$filter_name", $filter);
+        }
+      }

On the question of using hook_ENTITY_TYPE_presave() for this:

It's not a case of caring about default views as such, it's about default configuration in general, and having a standard way to handle this for backwards compatibility.

Using hook_ENTITY_TYPE_presave() looks heavy-handed, but I don't think it's that bad really:

- generally it's a couple of quick if checks
- it allows us to catch any case where a configuration entity might be saved via the API - i.e. created in hook_install() or manually using $entity->save() in test coverage and similar.
- we can remove all this code in 9.0.x

However it'd be good to make it clearer which bit is the bc-layer and which bit isn't, so people don't make the same mistake I just did.

edited to add:
While this has been discussed a few times (especially on the two issues that Lendude linked), the docs on https://www.drupal.org/docs/8/api/update-api/updating-configuration-in-d... don't mention this at all, so we should probably open a new task to update that documentation with the presave approach. While it's not perfect given it's only used for cases that need bc layers I do think it's OK if we document it better (or at least we should open a new generic issue for config entity updates/bc to try to replace the pattern with something better).

So looks like we have input here from the dblog subsystem maintainer (thanks @dagmar!), a Views maintainer (thanks @Lendude), and a Configuration system maintainer (@alexpott). I think the concern here is around the correct approach for the upgrade path? In #55 @Lendude says it seems nonideal but also that they didn't come up with anything better.

I've opened #2870724: Define and document best practices for configuration entity updates/bc layers to discuss the upgrade path approach more generally.

@catch++
thanks for the great summary and opening the issue.

@xjm++
And yes, hook_ENTITY_TYPE_presave() certainly beats doing nothing.

Took the 'Needs subsystem maintainer' tag off, @dawehners concern was with doing assertions before running updates in the update path test, and we are not doing that currently, so I assume that's ok.

(12:54:19) berdir: dagmar_drupal: what exactly is $view? config entities do not support the . syntax like config objects do?

OOOH, interesting, I had no idea.

1. +++ b/core/modules/dblog/dblog.install
   @@ -90,3 +92,20 @@ function dblog_schema() {
   +  \Drupal::service('module_handler')->resetImplementations();
   

   You can actually use \Drupal::moduleHandler here ...

2. +++ b/core/modules/dblog/dblog.module
   @@ -115,3 +116,75 @@ function dblog_form_system_logging_settings_alter(&$form, FormStateInterface $fo
   +    $view->set('display', $displays);
   

   Really good to know!

1. +++ b/core/modules/dblog/dblog.module
   @@ -115,3 +116,75 @@ function dblog_form_system_logging_settings_alter(&$form, FormStateInterface $fo
   +  $numeric_attributes_to_delete = [
   +    'set_precision',
   +    'precision',
   +    'decimal',
   +    'separator',
   +    'format_plural',
   +    'format_plural_string',
   +    'prefix',
   +    'suffix',
   +  ];
   ...
   +          // Delete all the attributes related to numeric fields.
   +          foreach ($numeric_attributes_to_delete as $key) {
   +            if (isset($field[$key])) {
   +              unset($displays[$display_name]['display_options']['fields'][$field_name][$key]);
   +            }
   +          }
   

   This could be less complex... just

   unset(
     $displays[$display_name]['display_options']['fields'][$field_name]['set_precision'],
     $displays[$display_name]['display_options']['fields'][$field_name]['precision'],
     $displays[$display_name]['display_options']['fields'][$field_name]['decimal'],
     $displays[$display_name]['display_options']['fields'][$field_name]['separator'],
     $displays[$display_name]['display_options']['fields'][$field_name]['format_plural'],
     $displays[$display_name]['display_options']['fields'][$field_name]['format_plural_string'],
     $displays[$display_name]['display_options']['fields'][$field_name]['prefix'],
     $displays[$display_name]['display_options']['fields'][$field_name]['suffix']
   );
   

   No loops, no ifs... unset doesn't fail if the thing doesn't exist and can do multiple at once.

2. +++ b/core/modules/dblog/dblog.module
   @@ -115,3 +116,75 @@ function dblog_form_system_logging_settings_alter(&$form, FormStateInterface $fo
   +            in_array($field['field'], ['wid', 'uid'])) {
   

   Should set strict to TRUE... its a third argument.

1. +++ b/core/modules/dblog/dblog.module
   @@ -115,3 +116,70 @@ function dblog_form_system_logging_settings_alter(&$form, FormStateInterface $fo
   +/**
   + * Implements hook_ENTITY_TYPE_presave() for views entities.
   + *
   + * This hook ensures there are no views based that are using a wrong plugin for
   + * wid and uid fields on views that use watchdog as base table.
   + */
   +function dblog_view_presave(ViewEntityInterface $view) {
   

   Do you mind adding some line of documention that this should be removed in Drupal 9 again?

2. +++ b/core/modules/dblog/src/Plugin/views/filter/DblogTypes.php
   @@ -0,0 +1,24 @@
   +   * {@inheritdoc}
   +   */
   +  public function getValueOptions() {
   +    if (!isset($this->valueOptions)) {
   +      $this->valueOptions = _dblog_get_message_types();
   +    }
   +    return $this->valueOptions;
   

   you could just use an options callback instead of introducing a PHP class, see \Drupal\views\Plugin\views\filter\InOperator::getValueOptions

May I ask why we moved from post-update hook to update hook?

AFAICS there are changes in views plugins and there is no schema change so Drupal is not broken only certain views are broken. Moreover, use of entity API is not a good idea in update hook that's why we introduced the post-update hooks so IMO we should use a post-update hook.

We can, but I'm deliberate doing this because we are trying to deprecate _dblog_get_message_types and the options callback doesn't support the use of services.

What about making this possible? If you use controller_resolver to make this possible.

Moreover, use of entity API is not a good idea in update hook that's why we introduced the post-update hooks so IMO we should use a post-update hook.

I agree, we should really not use the config entity API, it can causes arbitrary issues you can't see upfront.

@dagmar and @dawehner discussed about this specific issue:

We agreed on trying out the following idea:

* use hook_update_N
* use config api
* call to dblog_view_presave() directly

This ensure we don't use the entity api, while still working early.

I agree that code duplication here is not a bad thing. Its a once thing, which probably will not be adapted. If we want to adapt it we probably should rebuild it.

+++ b/core/modules/dblog/dblog.views.inc
@@ -20,11 +20,19 @@ function dblog_views_data() {
+  $data['watchdog']['table']['join'] = [
+    // Link ourselves to the {users_field_data} table.
+    'users_field_data' => [
+      'field' => 'uid',
+      'left_field' => 'uid',
+    ],
+  ];

This is actually not a bug and not needed at all. Given that we already have a relationship to user_field_data this adds confusion.
The relationship shows the user details of the log entry. Whereas this doesn't make sense why in a user view we want to show its log entries by default.

That's not true.
Please see following:

Thank you! @dagmar for fixing that.

+++ b/core/modules/dblog/tests/modules/dblog_test_views/test_views/views.view.dblog_integration_test.yml
@@ -0,0 +1,505 @@
+        name:

+++ b/core/modules/dblog/tests/src/Kernel/Views/ViewsIntegrationTest.php
@@ -18,41 +18,161 @@
+    // The uid relationship should now join to the {users_field_data} table.
+    $tables = array_keys($view->getBaseTables());
+    $this->assertTrue(in_array('users_field_data', $tables));
+    $this->assertFalse(in_array('users', $tables));

We already have a name field in the view and we have these asserts so we already have a test coverage for the bug described in #91. Back to RTBC.

1. This looking great. we just need a change record and for the deprecation notices to link to it (as per https://www.drupal.org/core/deprecation)

2. +++ b/core/modules/dblog/dblog.module
   @@ -115,3 +116,72 @@ function dblog_form_system_logging_settings_alter(&$form, FormStateInterface $fo
   + * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.
   

   Should link to the change record.

3. +++ b/core/modules/dblog/dblog.module
   @@ -115,3 +116,72 @@ function dblog_form_system_logging_settings_alter(&$form, FormStateInterface $fo
   +          @trigger_error("The numeric plugin for watchdog.$field_name field is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Must use standard plugin instead", E_USER_DEPRECATED);
   ...
   +          @trigger_error("The in_operator plugin for watchdog.type filter is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Must use dblog_types plugin instead", E_USER_DEPRECATED);
   

   These should point to the change record.

   It's great having these deprecation notices here because it means we'll know to remove it for 9.x

patch has been re-rolled, points have been changed as alexpott mentioned

Thanks, @dagmar. I have updated some wording and title for the change notice.

Committed ef0fe5d and pushed to 8.4.x. Thanks!

I've credited @jibran, @Lendude, @dawehner and @catch for reviewing the patch. @xjm and @tstoeckler thank you for your comments.

@akashkrishnan thanks for trying to address #98 in #99. I've not credited you because you didn't provide an interdiff and your patch was half formed. It's good practice to review your patches before uploading. A good indication that something has gone wrong when you are only making minor changes is patch size. @dagmar has provided you useful links. Thanks for contributing!