views_update_8500() inlines configuration changes instead of this being done on config save for bc

Re-exporting configuration after a core update is expected, I can't find the issue talking about that at the moment though. Re-exporting configuration after update so it matches the state in the database should be standard though - i.e. if a module in an update adds a new config entity, you'd need to re-export first to avoid deleting it when you import.

Exported configuration in modules ought to have been covered by the original patch I think, as an example see the discussion in #2784233: Allow multiple vocabularies in the taxonomy filter. Can't find a core example right now but they exist.

Thanks for reporting this! As far as I know, we haven't gotten any other reports of this breaking for people, and it's been 5 months since 8.5.0 was released, so I'm very curious about more detailed specifics on how to reproduce a breaking condition.

I wouldn't expect modules to have to make changes to their exported views?

No generally you shouldn't and we have other updates where the configuration change is made on configuration save rather in the update directly (then the update just loads and resaves all the affected configuration objects) - so that does seem like an oversight in the original patch (from a quick read over the old issue, I'm not properly online at the moment).

I agree with #6. We should have triggered the update on config save. In fact I think that this scenario is common enough that we should consider ways to make this really simple and add documentation about how to do this correctly.

Title change and issue summary update.

Tagging as a requirement for Drupal 9.0-beta1.

Here is a start, update path test is not passing yet.

Done:
* refactored views_view_presave so it doesn't become a mess when doing multiple config updates here
* Moved the update logic to a post_update hook that triggers a save and views_view_presave does the rest

I think the remaining problem lies with:
// Beside of updating the field and plugin ID we also need to truncate orphan
// keys so the configuration applies to the config schema.
// We cannot do that inline in the other code, due to caching issues with
// typed configuration.
The original update just triggered a save twice. That option is not available to us in views_view_presave, so that needs work, but posting progress.

Going to work on this.

Sorry, I grabbed the wrong issue.

Working on this

This should hopefully fix test failures, I'm now working on fixing an issue that wasn't captured by the previous test coverage.

And this should fix operator handling + making the cleanup logic more defensive.

Wrong interdiff, sorry

Minor clean-up

Factoring out the presave logic into a helper seems good given the amount of logic in there now.

+++ b/core/modules/views/views.install
@@ -402,133 +402,6 @@ function views_update_8201() {
-  }
+  // This update has been replaced by
+  // views_post_update_field_names_for_multivalue_fields().
 }

We'll need a 9.0.x patch to add this to views_removed_post_updates()

1. +++ b/core/modules/views/src/ViewsConfigUpdater.php
   @@ -0,0 +1,374 @@
   +
   +            // Get the actual table, as well as the column for the main property
   +            // name so we can perform an update later on the views.
   +            $table_name = $table_mapping->getFieldTableName($field_name);
   

   Minor: the 'later on' used to be in the update function, but now it's in a different method in this class - could be reworded to say ::processMultivalueBasefieldUpdate?

2. +++ b/core/modules/views/src/ViewsConfigUpdater.php
   @@ -0,0 +1,374 @@
   +   *   The handler cleanup info.
   +   */
   +  protected function queueHandlersCleanup($id, array $handler_cleanup_info) {
   +    $info = &$this->multivalueBaseFieldsUpdateInfo['cleanup'];
   +    if (!$info) {
   +      drupal_register_shutdown_function(function () {
   +        try {
   +          $this->doHandlersCleanup();
   +        }
   

   Oof. So we have to run this on presave, and we also have to run this during the update. I was thinking whether we could move this to a queue instead, but that'd leave things broken until the queue runs.

This should address #25.

@catch:

I was thinking whether we could move this to a queue instead, but that'd leave things broken until the queue runs.

I had the same idea and reached the same conclusion. Maybe we could create queue items and try to process them in the shutdown function leaving the regular cron run as a fallback, thoughts?

Would be nice if we had #1189464: Add an 'instant' queue runner but without that seems like extra work to try to fire off the queue items in shutdown. Could use a third opinion maybe.

OK apologies I was wrong about #25. Because this is a new post update in 8.8.x, we can't remove it from 9.0.x. So... instead of adding it to hook_removed_post_updates() we'd instead need to add a patch with the post update itself. Not sure whether we should also forward-port the bc layer, or just have the post update be empty.

Any site updating to 8.8 will run the new post-update, sites that have already updated could I suppose have config that still needs to be updated, but that means leaving the presave logic in until 10.0.x. It is probably more correct to have the bc layer in 10.0.x for any config that was somehow left un-updated for three years, but it might be unnecessary complexity and maintenance for three years too.

Either option will mean the test passes.

Here's a D9 version of the patch.

Not pretty but I'd tend to agree that keeping the BC layer in until 10.0.x is desirable.

+++ b/core/modules/views/src/ViewsConfigUpdater.php
@@ -0,0 +1,375 @@
+
+  /**
+   * Add additional settings to the entity link field.
+   *
+   * @param \Drupal\views\ViewEntityInterface $view
+   *   The View to update.
+   */
+  public function updateEntityLinkUrl(ViewEntityInterface $view) {
+    $displays = $view->get('display');
+    $changed = FALSE;

We should be able to remove this one in 9.x. I think that's an older bc layer for an already-removed post update.

Rright!

Not sure how to deal with test coverage. Should we restore EntityViewsMultiValueBaseFieldDataUpdateTest and its fixtures?

OK that is really confusing, even though we removed the post update for entity links, we apparently still have test coverage for the bc layer, and that bc layer was not removed from 9.0.x...

Should we restore EntityViewsMultiValueBaseFieldDataUpdateTest and its fixtures?

That seems very unfortunate - it would require a pre-8.8 database dump. I think we should either add a new test with an 8.8 dump, or just skip the test coverage in Drupal 9.

@catch and I agreed that the 9.0.x-specific test coverage can be a followup, so that this has a better chance of landing before beta1. The 8.9/8.8 coverage should be sufficient in the meanwhile.

This removes the legacy test code.

Follow-up created at #3121008: Add test coverage for ViewsConfigUpdater.

+++ b/core/modules/views/src/ViewsConfigUpdater.php
@@ -0,0 +1,325 @@
+   */
+  protected function queueHandlersCleanup($id, array $handler_cleanup_info) {
+    $info = &$this->multivalueBaseFieldsUpdateInfo['cleanup'];
+    if (!$info) {
+      drupal_register_shutdown_function(function () {
+        try {
+          $this->doHandlersCleanup();
+        }
+        // Avoid breaking other shutdown functions.
+        catch (\Exception $e) {
+          watchdog_exception('views', $e);
+        }
+      });

So we still have this, but I just cannot think of another idea. I think we should go ahead with it. The likelihood of this code being run is increasingly low as time goes on.

RTBC for #35 for 9.0.x

Patch in #26 is good for 8.9.x, 8.8.x and 8.7.x (going to kick off a test against the last one).

1. +++ b/core/modules/views/views.module
   @@ -822,49 +823,9 @@ function views_view_delete(EntityInterface $entity) {
   -  $displays = $view->get('display');
   -  $changed = FALSE;
   -  foreach ($displays as $display_name => &$display) {
   -    if (isset($display['display_options']['fields'])) {
   -      foreach ($display['display_options']['fields'] as $field_name => &$field) {
   -        if (isset($field['plugin_id']) && $field['plugin_id'] === 'entity_link') {
   -          // Add any missing settings for entity_link.
   -          if (!isset($field['output_url_as_text'])) {
   -            $field['output_url_as_text'] = FALSE;
   -            $changed = TRUE;
   -          }
   -          if (!isset($field['absolute'])) {
   -            $field['absolute'] = FALSE;
   -            $changed = TRUE;
   -          }
   -        }
   -        elseif (isset($field['plugin_id']) && $field['plugin_id'] === 'node_path') {
   -          // Convert the use of node_path to entity_link.
   -          $field['plugin_id'] = 'entity_link';
   -          $field['field'] = 'view_node';
   -          $field['output_url_as_text'] = TRUE;
   -          $changed = TRUE;
   -        }
   -      }
   -    }
   -    if (isset($display['display_options']['filters'])) {
   -      foreach ($display['display_options']['filters'] as $filter_name => &$filter) {
   -        if (!isset($filter['expose']['operator_limit_selection'])) {
   -          $filter['expose']['operator_limit_selection'] = FALSE;
   -          $changed = TRUE;
   -        }
   -        if (!isset($filter['expose']['operator_list'])) {
   -          $filter['expose']['operator_list'] = [];
   -          $changed = TRUE;
   -        }
   -      }
   -    }
   -  }
   -  if ($changed) {
   -    $view->set('display', $displays);
   -  }
   

   So I have a concern about removing this. This has been silently fix views config provided by modules and configuration imports in Drupal 8. It's not emitting a silenced deprecation error so it's very possible a module won't know it needs to update its configuration. Are we sure we want to remove this and not change this to add a deprecation error?

   I guess I'm asking why we don't have ViewsConfigUpdater::updateEntityLinkUrl() in the D9 version and I think it should trigger a deprecation.

2. +++ b/core/modules/views/src/ViewsConfigUpdater.php
   @@ -0,0 +1,375 @@
   +  /**
   +   * Add additional settings to the entity link field.
   +   *
   +   * @param \Drupal\views\ViewEntityInterface $view
   +   *   The View to update.
   +   */
   +  public function updateEntityLinkUrl(ViewEntityInterface $view) {
   

   Also this method doesn't only fix entity link stuff. The operator_limit_selection and operator_list are different things (less important mind you).

3. +++ b/core/modules/views/src/ViewsConfigUpdater.php
   @@ -0,0 +1,325 @@
   +  public function updateFieldNamesForMultivalueBaseFields(ViewEntityInterface $view) {
   

   I think if this method results in a view changing outside of the update path then it should trigger a deprecation. So people can discover that they need to update their module's views. I think this can be a follow-up that we add in 9.1.x since we're going to support this through to Drupal 10. But we need to ensure that doing this is possible. I think we might need to use the MAINTENANCE_MODE constant for this.

4. +++ b/core/modules/views/src/ViewsConfigUpdater.php
   @@ -0,0 +1,325 @@
   +            $result = call_user_func_array($callback, [&$data, $display_id, $handler_type_singular, $key]);
   +            if ($result === FALSE) {
   +              return;
   +            }
   

   So as far as I can see we have two callbacks passed into this method - one that's always going to return FALSE and one that's always going to return NULL. Which seems really odd behaviour. I think we need to document what the return value of the callback means. And I think we should say that returning TRUE causes an early return. And anything else means that the view will continue to be process looking for the next field.

Won't have time to work on this until tonight, unassigning in case someone else wishes to pick this up.

This should address @alexpott's review: it reverts the BC layer removal and adds conditional deprecation + the related test coverage. I decided to add a kernel test so we have starting point to provide D9 test coverage in #3121008: Add test coverage for ViewsConfigUpdater.

And the usual minor fixes after the self-review.

And this should fix the new deprecation errors.

Given that core itself is triggering deprecations, we even have formal proof that @alexpott was right in #40 :)

Ugh, I just realized that I did not address #40.3 and #40.4.

This also fixes a namespace error in the new test class.

Sorry for the noise :(

🤦‍♂️

1. +++ b/core/modules/views/src/ViewsConfigUpdater.php
   @@ -0,0 +1,424 @@
   +
   +  /**
   +   * Whether deprecations should be triggered.
   +   *
   +   * @var bool
   +   */
   +  protected $deprecationsEnabled = TRUE;
   +
   

   Do we need this? Looks like it's for test coverage, but would think legacy test coverage with expectedDeprecation would be enough.

2. +++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
   @@ -172,6 +172,9 @@ public static function getSkippedDeprecations() {
          "The \"PHPUnit\Framework\TestCase::__construct()\" method is considered internal This method is not covered by the backward compatibility promise for PHPUnit. It may change without further notice. You should not extend it from \"Drupal\FunctionalTests\Update\UpdatePathTestBase\".",
   +      // The following deprecation were not added as part of the original issue
   +      // and thus were not addressed in time for the 9.0.0 release.
   +      'ViewsConfigUpdater::updateEntityLinkUrlAndExposedFilterOperator() is deprecated in drupal:8.9.0 and is removed from drupal:10.0.0. Module-provided Views configuration should be updated to accommodate the changes described at http://drupal.org/node/2857891.',
        ];
      }
   

   If core is triggering this deprecation message, we should have a follow-up to resave/re-export those views and remove skipping it here.

I agree with #49. Here are some other thoughts, but the majority of the patch is sound

1. +++ b/core/modules/views/src/ViewsConfigUpdater.php
   @@ -0,0 +1,424 @@
   + * @internal
   

   Missing explanation of why it is internal (yes not all old instance have that, but it's best practice to have a line after this one, see \Drupal\Tests\Listeners\DeprecationListenerTrait for an example)

2. +++ b/core/modules/views/src/ViewsConfigUpdater.php
   @@ -0,0 +1,424 @@
   +  /**
   +   * @var \Drupal\Core\Entity\EntityFieldManagerInterface
   +   */
   +  protected $entityFieldManager;
   

   Nit: missing oneliner

3. +++ b/core/modules/views/src/ViewsConfigUpdater.php
   @@ -0,0 +1,424 @@
   +   * @see http://drupal.org/node/2857891
   

   Super-nit, phpcs is complaining that this should be https://www. Some are missing the https, some are missing the www (idk why the rule cares that much)

4. +++ b/core/modules/views/src/ViewsConfigUpdater.php
   @@ -0,0 +1,424 @@
   +      if (isset($display['display_options']['fields'])) {
   +        foreach ($display['display_options']['fields'] as $field_name => &$field) {
   +          if (isset($field['plugin_id']) && $field['plugin_id'] === 'entity_link') {
   +            // Add any missing settings for entity_link.
   +            if (!isset($field['output_url_as_text'])) {
   +              $field['output_url_as_text'] = FALSE;
   +              $changed = TRUE;
   +            }
   +            if (!isset($field['absolute'])) {
   +              $field['absolute'] = FALSE;
   +              $changed = TRUE;
   +            }
   +          }
   +          elseif (isset($field['plugin_id']) && $field['plugin_id'] === 'node_path') {
   +            // Convert the use of node_path to entity_link.
   +            $field['plugin_id'] = 'entity_link';
   +            $field['field'] = 'view_node';
   +            $field['output_url_as_text'] = TRUE;
   +            $changed = TRUE;
   +          }
   +        }
   +      }
   

   This is 1:1 copied from views_post_update_entity_link_url()

   It's also 1:1 copied from where you moved it from, which means it was always duplicated since being introduced in #2810097: Allow views to provide the canonical entity URL of all entities, not just nodes.

   Which makes me wonder, why not just call the post_update function directly? Since it's BC anyway and it seems fraught to have this relatively delicate code be duplicated...

5. +++ b/core/modules/views/src/ViewsConfigUpdater.php
   @@ -0,0 +1,424 @@
   +      if (isset($display['display_options']['filters'])) {
   +        foreach ($display['display_options']['filters'] as $filter_name => &$filter) {
   +          if (!isset($filter['expose']['operator_limit_selection'])) {
   +            $filter['expose']['operator_limit_selection'] = FALSE;
   +            $changed = TRUE;
   +          }
   +          if (!isset($filter['expose']['operator_list'])) {
   +            $filter['expose']['operator_list'] = [];
   +            $changed = TRUE;
   +          }
   +        }
   +      }
   +    }
   

   Same goes for this, except it's views_post_update_limit_operator_defaults() and was added in #1886018: Make it possible to configure exposed filter operators

   And if the answer is no, can they at least @see each other?

6. +++ b/core/modules/views/src/ViewsConfigUpdater.php
   @@ -0,0 +1,424 @@
   +  protected function processMultivalueBaseFieldUpdateHandlers(ViewEntityInterface $view, callable $callback) {
   

   This is a nice way to do this. Kudos

7. +++ b/core/modules/views/src/ViewsConfigUpdater.php
   @@ -0,0 +1,424 @@
   +      case '=':
   +        return 'or';
   +      case '!=':
   +        return 'not';
   +      default:
   

   Nit: missing blank line after the returns.

This should address #50.

@catch, #49:

1: Actually ::setDeprecationsEnabled() is used in views.post_update.php to avoid triggering deprecations during the update, as suggested by @alexpott.
2: Sure, I will create the follow-up next.

Follow-up created at #3123930: Fix test views to avoid triggering deprecations for CR #2857891 and #2869168.

These fixes a couple of whitespace issues.

Here's an idea of how not to do the double save of views and the shutdown function. So far for 8.9.x only... posting this to see what @plach and @catch think...

Thanks @alexpott, that's great!

This exploits #53 to completely unify the behavior of the various updates, increase code reuse, and avoid duplicate processing.

The D9 patch is now way bigger because it relies on two additional test views that were previously removed from the 9.0.x branch.

This slightly improves performance and performs some minor cleanups.

The D9 patch in #55 was messed up, the interdiff (roughly) applies to both versions.

If the latest patch you reviewed was #53, it would probably be faster to review the patches rather than the interdiffs...

And finally some test coverage improvements.

This looks much nicer now.

+++ b/core/modules/views/src/ViewsConfigUpdater.php
@@ -0,0 +1,472 @@
+    if ($changed && !$deprecations_triggered) {
+      $deprecations_triggered = TRUE;
+      @trigger_error('The views entity link url update is deprecated in drupal:8.9.0 and is removed from drupal:10.0.0. Module-provided Views configuration should be updated to accommodate the changes described at https://www.drupal.org/node/2857891.', E_USER_DEPRECATED);
+    }
...
+    if ($changed && !$deprecations_triggered) {
+      $deprecations_triggered = TRUE;
+      @trigger_error('The views operator defaults update is deprecated in drupal:8.9.0 and is removed from drupal:10.0.0. Module-provided Views configuration should be updated to accommodate the changes described at https://www.drupal.org/node/2869168.', E_USER_DEPRECATED);
+    }
+
...
+    if ($changed && !$deprecations_triggered) {
+      $deprecations_triggered = TRUE;
+      @trigger_error('The views multivalue base field update is deprecated in drupal:8.9.0 and is removed from drupal:10.0.0. Module-provided Views configuration should be updated to accommodate the changes described at https://www.drupal.org/node/2900684.', E_USER_DEPRECATED);
+    }

I think that this should say which view. A site can have hundreds of views... It means we need to add the affect view to $this->triggeredDeprecations but I think that's fine... one message per view seems the right level of detail.

Yes please! That mimics the follow-up we did for theme function names #3120954: Add function name to the deprecation message about theme functions

This should address #59. As discussed with @alexpott in Slack, we are removing deprecations from 8.9.0, since we don't support dynamic deprecation messages there. The D8 and D9 versions of the patch have diverge quite noticeably at this point, so I'm providing two separate interdiffs.

And the missing interdiff...

#61 is a huge improvement on older versions of the patches, glad we could remove the double save in the end.

Since we have to keep the bc layer in until Drupal 10, removing the deprecations from the 8.9 patch seems OK.

I was thinking that we could even restore deprecations in 8.9 and log the affected view instead as a fallback, but more than happy to leave things as they are :)

I really don't think it matters in the end, but I found it odd that the method signatures differed between 8.9 and 9.0.
Two of the process methods are missing $view (which would be needed if #64 is done)

processEntityLinkUrlHandler()
processOperatorDefaultsHandler()

Other than that, the differences between the two patches are correct, and the similarities between the two are great.

I'm going to RTBC as-is for now. If you want to add $view to keep them similar, I can re-RTBC after that addition.

Thanks!

-   * @param string
+   * @param string $view_id

Just a tiny change, keep it as RTBC

Failures seem unrelated and the re-testing passed, so setting back to RTBC.

Committed the 9.x patch to 9.1.x and 9.0.x, and the 8.x patch to 8.9.x and 8.8.x

Added release notes snippets and tags.

Missed one.

Polishing the release note a little.