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
Comment | File | Size | Author |
---|---|---|---|
#54 | interdiff-2407853-44-47.txt | 675 bytes | anhtq |
#54 | 2407853-47.patch | 11.91 KB | anhtq |
#47 | 2407853-47.patch | 11.9 KB | alexpott |
#47 | 44-47-interdiff.txt | 672 bytes | alexpott |
#44 | 2407853-44.patch | 11.72 KB | alexpott |
Comments
Comment #1
vladan.me CreditAttribution: vladan.me commentedComment #2
vladan.me CreditAttribution: vladan.me commentedComment #3
dawehnerCan you please quickly explain why they have to be optional? How can
$term
not exist?Comment #4
BerdirBecause 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?
Comment #5
BerdirUpdating the a major bug, changing issue title to something that reflects the real issue.
Comment #6
xjmAgreed on major bug (since it's a content dependency that cannot be deployed properly). Adding the parent issue.
Comment #7
xjmComment #8
xjmAlso 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.
Comment #9
xjmOh and the code belongs to Taxonomy.
Comment #10
xjmI'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.
Comment #11
dawehnerThe 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.
Comment #12
BerdirNo, 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 :)
Comment #13
dawehnerFair.
In general this is no longer in the state of needs review, its more like active.
Comment #14
jhedstromIf 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?Comment #15
BerdirWe 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).
Comment #16
xjmI did not take a look at this; I worked on menu stuff that week and month. Sorry.
Comment #17
catchComment #18
BerdirReuploading 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).
Comment #19
dawehner@berdir
I'm curious, is there some quick way to test that bit of change?
Comment #20
BerdirCreate 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.
Comment #21
dawehnerAlright
Comment #22
aerozeppelin CreditAttribution: aerozeppelin commentedAn attempt to write tests following the steps mentioned in #20.
Comment #24
xjm@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!
Comment #25
Wim LeersHow does this fix
? :OI think an explanatory comment is necessary here, unfortunately.
Comment #26
aerozeppelin CreditAttribution: aerozeppelin commented@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.
Comment #27
dawehnerI'm sorry, but this is the exact purpose of
getConfigDependencyName()
andkey()
, so do you really suggest to document the one function call here?Comment #28
BerdirNo, 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.
Comment #29
Wim LeersI'm saying it's not clear why
if ($term) {
is necessary.Comment #30
BerdirBecause 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.
Comment #31
Wim LeersAlright. Can we then say:
Because that was not at all obvious to me, and apparently is a deeper flaw.
Comment #33
serg2 CreditAttribution: serg2 commentedI 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.
Comment #34
davemybes CreditAttribution: davemybes commentedJust 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.
Comment #35
alexpottSo 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.
Comment #36
alexpottHere's the patch from the other issue. No interdiff cause different patch.
Comment #37
alexpottComment #38
alexpottI'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.Comment #39
dawehnerNitpick: You would just need 'node' here to install
Comment #40
alexpott@dawehner methinks you added that code :)
Comment #41
alexpottTicking the credit box for @dawehner because the patch in #36 is largely his.
Comment #42
dawehnerHa, well yeah I don't care that much to be able to post a patch with less modules :)
Comment #43
xjmThe 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.
Comment #44
alexpottHere'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.
Comment #45
dawehnerPersonally 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.
I'm wondering whether its worth to just save views which have this filter configured ...
Comment #46
alexpott@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.
Comment #47
alexpottPatch 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.Comment #48
dawehnerYeah 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.
Comment #51
catchLet's add a trait later if this gets annoying.
Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!
Comment #53
cilefen CreditAttribution: cilefen as a volunteer commentedComment #54
anhtq CreditAttribution: anhtq at Google Code-In commented( For Google Code-In )
Including the updated patch in #47 and also the interdiff comparing the patches in #44 and #47
Comment #55
anhtq CreditAttribution: anhtq at Google Code-In commented