Problem/Motivation

In the issue Add content & config entity dependencies to views are added dependencies where Views should depend on all the things (config, modules, and content) that if they disappear would break the view but content dependencies *must* be optional. If a taxonomy term is deleted the view still maintains a reference to the term and when it calculates dependencies this fails with

Error: Call to a member function getConfigDependencyKey() on null in
/usr/local/var/vhosts/unidesk.com/acquia/unidesk8/docroot/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php,

Why is this critical? This is affecting users upgrading to 8.1.0 because there is an update that re-saves all views - which recalculates dependencies.

Proposed resolution

Don't break if the term does not exist.

On affected sites where views_post_update_field_formatter_dependencies there is a possible manual work around - you can edit the view and resave the taxonomy filter and then resave the view - this will fix the reference to the non-existent term and the update will proceed without failing.

Remaining tasks

It is not necessary to add a new post update since on affected sites views_post_update_field_formatter_dependencies will not have completed and it will still show as an available update.

User interface changes

None

API changes

None

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vladan.me’s picture

Status: Active » Needs review
FileSize
736 bytes
vladan.me’s picture

Title: Content dependendies of views must be optional » Content dependencies of views must be optional
dawehner’s picture

Can you please quickly explain why they have to be optional? How can $term not exist?

Berdir’s picture

Because that is the rule for content dependencies :)

Code must not fail if a content dependency is missing. Core has no way to deploy content, and we will add a way for contrib to jump in, but that will happen after config was imported.

That said, the code above will break that, because unless we save the view again, that dependency is no longer there. I honestly don't know how to fix that other than not storing ID's but UUID's in configuration, so that we don't have to load the content to add the dependencies. I think you are working on that?

Berdir’s picture

Title: Content dependencies of views must be optional » TaxonomyIndexTid stores selected terms with the ID instead of UUID
Priority: Normal » Major
Issue tags: +Configuration system, +Needs issue summary update

Updating the a major bug, changing issue title to something that reflects the real issue.

xjm’s picture

Agreed on major bug (since it's a content dependency that cannot be deployed properly). Adding the parent issue.

xjm’s picture

xjm’s picture

Issue tags: +D8 upgrade path

Also not an upgrade path blocker, since it's major, but existing views with this filter will break minorly after we change the schema here for a UUID.

xjm’s picture

Component: views.module » taxonomy.module

Oh and the code belongs to Taxonomy.

xjm’s picture

Assigned: Unassigned » xjm

I'll take a look into this as it has a lot in common with #2341357: Views entity area config is not deployable and missing dependencies.

dawehner’s picture

The proper way to fix it is to use $term_storage->loadMultiple() and a foreach, then you simply don't need an if statement in the first place.

Berdir’s picture

No, the only proper way is not needing a load at all, because losing a content entity dependency when the content entity doesn't exist defeats the whole purpose of having it :)

dawehner’s picture

Status: Needs review » Active

Fair.

In general this is no longer in the state of needs review, its more like active.

jhedstrom’s picture

If I'm understanding the related fix over in #2341357: Views entity area config is not deployable and missing dependencies, it uses calculateDependencies(). If that same fix were applied here, wouldn't every view with exposed term filters (or arguments, etc) be marked as incompatible every time a term was deleted?

Berdir’s picture

We don't treat content dependences the same way we do configuration dependencies. They are supposed to be optional.

So you can deploy a vew that depends on a non-existing term and it shouldn't produce any errors.

The problem here is that when we store it by ID, then we have no possibility to recalculate the UUID if the ID is missing, so we are forced to remove it as a dependency, which however removes the possibly to find and create those missing dependencies (for a contrib module).

xjm’s picture

Assigned: xjm » Unassigned

I did not take a look at this; I worked on menu stuff that week and month. Sorry.

catch’s picture

Title: TaxonomyIndexTid stores selected terms with the ID instead of UUID » TaxonomyIndexTid Views plugin stores selected terms with the ID instead of UUID
Berdir’s picture

Reuploading patch.

The problem discussed above is not specific to this, but there's no other way right now. See #2597854: Content dependencies lost on export and importing again if the content entities do not exist.

I still think it would be good to get this in as-is, until the other issue is resolved. We just had another case when this broke the upgrade path as somehow the config referenced a term that didn't exist (anymore).

dawehner’s picture

@berdir
I'm curious, is there some quick way to test that bit of change?

Berdir’s picture

Create a view with a filter on a given taxonomy term, then delete the term, then try to save the view. Should fail with that error I think.

dawehner’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Alright

aerozeppelin’s picture

An attempt to write tests following the steps mentioned in #20.

Status: Needs review » Needs work

The last submitted patch, 22: 2407853-fail.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review

@aerozeppelin, for future reference, when you attach your patches to the comment, upload the test-only one first followed by the full one to avoid the bot marking the issue NW. :) Thanks for your work on the test!

Wim Leers’s picture

+++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
@@ -394,7 +394,9 @@ public function calculateDependencies() {
-      $dependencies[$term->getConfigDependencyKey()][] = $term->getConfigDependencyName();
+      if ($term) {
+        $dependencies[$term->getConfigDependencyKey()][] = $term->getConfigDependencyName();
+      }

How does this fix TaxonomyIndexTid Views plugin stores selected terms with the ID instead of UUID? :O

I think an explanatory comment is necessary here, unfortunately.

aerozeppelin’s picture

@xjm Thank you for your comment. :D I knew something was wrong when i uploaded the patch, but did not know how to remedy it.

dawehner’s picture

I think an explanatory comment is necessary here, unfortunately.

I'm sorry, but this is the exact purpose of getConfigDependencyName() and key(), so do you really suggest to document the one function call here?

   *
   * Used to supply the correct format for storing a reference targeting this
   * entity in configuration.
Berdir’s picture

No, what he's saying is that we still don't store the dependencies with the UUID instead of the ID.

However, that wouldn't actually help us, as it would't solve #2597854: Content dependencies lost on export and importing again if the content entities do not exist, since we also need the bundle to generate the dependency key without loading the entity.

Wim Leers’s picture

I'm saying it's not clear why if ($term) { is necessary.

Berdir’s picture

Because when you deploy a view and that content doesn't exist yet, your config import process dies with a fatal error. Or install, if you use this with default_content or so.

This issue just fixes that fatal.

The unfortunate side effect is that this will then run into the same problem as #2597854: Content dependencies lost on export and importing again if the content entities do not exist explains, that your dependency is then lost. But we can't fix that here, so getting rid of the fatal is better than nothing.

Wim Leers’s picture

Alright. Can we then say:

// Only add a content dependency if the content exists.
// @todo Fix as part of https://www.drupal.org/node/2597854

Because that was not at all obvious to me, and apparently is a deeper flaw.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

serg2’s picture

I was unable to update from 8.0.6 to 8.1.0 without applying the patch at #22. I opened up #2709923: Fatal error: Call to a member function getConfigDependencyKey() when updating database to 8.0.6 to 8.1.0 which I will now close as a duplicate of this.

davemybes’s picture

Status: Needs review » Reviewed & tested by the community

Just hit this going from 8.0.5 to 8.1.0 (also tried updating to 8.0.6 first). Patch in #22 works great! Resaving the View that uses terms had no effect - the patch was the only way through the update.

alexpott’s picture

Priority: Major » Critical
Status: Reviewed & tested by the community » Needs review

So there is a better patch on #2613614: Fix the calculation of dependencies in TaxonomyIndexTid filter but that is a new issue so I'm going to mark it as duplicate of this one.

Given the number of people hitting this in the upgrade path I'm going to mark this as critical.

alexpott’s picture

Here's the patch from the other issue. No interdiff cause different patch.

alexpott’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update, -Needs tests
alexpott’s picture

Issue summary: View changes

I've done some update testing. If a site is affected then views_post_update_field_formatter_dependencies will never complete and therefore it'll still be showing as available to run on updates.php. There I don't think we need to introduce anymore post updates in this patch to fix the critical upgrade path issue.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyIndexTidFilterTest.php
@@ -0,0 +1,97 @@
+   */
+  public static $modules = ['taxonomy', 'taxonomy_test_views', 'views', 'node'];
+

Nitpick: You would just need 'node' here to install

alexpott’s picture

@dawehner methinks you added that code :)

alexpott’s picture

Ticking the credit box for @dawehner because the patch in #36 is largely his.

dawehner’s picture

Ha, well yeah I don't care that much to be able to post a patch with less modules :)

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Triaged D8 critical, +Needs upgrade path

The core committers agreed that this is critical but we need a repeat of the post-update to resave the views (and maybe upgrade path testing). We don't have an 8.0.x->8.1.x fixture/base test yet.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: -Needs upgrade path
FileSize
4.53 KB
11.72 KB

Here's an upgrade path and a kinda test for it. Rather than introduce full upgrade path testing here the test just ensures that the post update works as expected.

dawehner’s picture

  1. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyIndexTidFilterTest.php
    @@ -0,0 +1,156 @@
    +  /**
    +   * Tests post update function fixes dependencies.
    +   *
    +   * @see views_post_update_taxonomy_index_tid()
    +   */
    +  public function testPostUpdateFunction() {
    

    Personally for me an explicit test is kinda enough. It would help if we would have a helper trait or so to execute a specific post update function, so we have more trust that the integration bits are certainly done properly.

  2. +++ b/core/modules/views/views.post_update.php
    @@ -159,3 +159,22 @@ function views_post_update_field_formatter_dependencies() {
    +  $views = View::loadMultiple();
    +  array_walk($views, function(View $view) {
    +    $view->save();
    +  });
    +}
    

    I'm wondering whether its worth to just save views which have this filter configured ...

alexpott’s picture

@dawehner I wondered that too but then I couldn't think of an easy reliable way of doing that. I guess what we could do is call getDependencies(), calculateDependencies() and then only save those that have changed.

alexpott’s picture

Patch implements #46.

Re #45.1 I'm not sure a trait is worth it. All it is going to be wrapping is \Drupal::moduleHandler()->loadInclude('views', 'post_update.php');. The test is just directly calling the method - if the code was wrong then the function just would not exist.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Re #45.1 I'm not sure a trait is worth it

Yeah I'm not fighting for it. I was just thinking about it on the longrun, when we apply this pattern in more places.

The strict comparison is not a problem, because a) we just save more often potentially and b) the order is guaranteed and b) we just use strings in dependencies.

  • catch committed 82f9f8c on 8.2.x
    Issue #2407853 by alexpott, aerozeppelin, Berdir, vladan.me, dawehner:...

  • catch committed a61d36e on 8.1.x
    Issue #2407853 by alexpott, aerozeppelin, Berdir, vladan.me, dawehner:...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Let's add a trait later if this gets annoying.

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Status: Fixed » Closed (fixed)

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

anhtq’s picture

FileSize
11.91 KB
675 bytes

( For Google Code-In )
Including the updated patch in #47 and also the interdiff comparing the patches in #44 and #47

anhtq’s picture