Problem/Motivation

When you implement hook_views_invalidate_cache(), this hook is not invoked when a view is deleted.

In views.api.php you can find the following example hook that states that this hook is invoked when a view is enabled, disabled, created, updated or deleted. Which is not the case.

/**
 * Allow modules to respond to the invalidation of the Views cache.
 *
 * This hook will fire whenever a view is enabled, disabled, created,
 * updated, or deleted.
 *
 * @see views_invalidate_cache()
 */
function hook_views_invalidate_cache() {
  \Drupal\Core\Cache\Cache::invalidateTags(['views']);
}

Steps to reproduce

Proposed resolution

Invoke the hook when a view is deleted.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#5 3268872-5-test-only.patch1.3 KBjeroent

Issue fork drupal-3268872

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

JeroenT created an issue. See original summary.

JeroenT opened merge request !1956

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Title: hook_views_invalidate_cache not called when a view is deleted » hook_views_invalidate_cache() not called when a view is deleted
Issue summary: View changes
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Active » Needs review
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
StatusFileSize
new 3268872-5-test-only.patch1.3 KB
spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs review » Reviewed & tested by the community

- MR fixes hook_views_invalidate_cache() not being called when a view is deleted.
- MR includes test to prove the above
- Issue contains test-only patch with the above test that fails without the changes in the MR
- Code changes look good to me
- MR makes TestBot happy

RTBC for me.

spokje’s picture

spokje
Primary language Dutch
commented

Re-queue-ed a test on the MR so that will be the latest test known to Testbot and prevent TestBot from retesting the fail-patch every two days.

INSTA-EDIT (should really drink more coffee before starting hitting the "Save"-button on d.o. issues): The fail-patch Custom Commands failures on 10.0.x-dev with PHP8.0 are because that branch only supports PHP8.1

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented

Thanks for the review @Spokje!

It's strange that the first commit, which contains only the test changes, passed the tests and the patch is failing.

spokje’s picture

spokje
Primary language Dutch
commented

It's strange that the first commit, which contains only the test changes, passed the tests and the patch is failing.

As far as I can see, the first commit on the MR already contained the actual fix in core/modules/views/src/Entity/View.php, thus making it pass: https://git.drupalcode.org/project/drupal/-/merge_requests/1956/diffs?diff_head=true

That _is_ weird: https://git.drupalcode.org/project/drupal/-/merge_requests/1956/diffs?di...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 3268872-5-test-only.patch, failed testing. View results

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Reviewed & tested by the community

Fail test gonna fail, back to RTBC whilst we await the MR being retested.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

We need to consider what to do with views_view_delete() - that has logic to only rebuild routes if the view has routed displays. We can wither choose to remove it - or we can choose to not call views_invalidate_cache() and call 

  // Allow modules to respond to the Views cache being cleared.
  $module_handler->invokeAll('views_invalidate_cache');

from views_view_delete()

I'm not sure what's best here.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented

I rebased the MR to 9.5.x.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Needs work » Needs review

I was wondering, since views_invalidate_cache() contains some code to clear the block cache and rebuild the views blocks, that when you delete a view, the blocks on the block layout page weren't updated.

But that seems to work fine, so both solutions probably work.

I updated the MR and added the call to $module_handler->invokeAll('views_invalidate_cache'); in views_view_delete().

I'm actually also not sure what the best approach is here. Having 1 function that contains this logic, or copying a part of the logic for performance reasons when saving a view...

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented

@JeroenT asked for my 2ct, so here we go:

I'm not a fan of having entity hooks in the module that implements said entity, so getting rid of views_view_delete() sounds good to me (even if @alexpott and @dawehner apparently likeed this solution more in #2596345: Deleting a view should rebuild the routes ;) ). And calling a hook from inside a hook I also don't like, and invoking a hook from two different places I also don't like.

I would prefer just the added overhead of the extra rebuilding by adding views_invalidate_cache to the postDelete method, since that would be the clearer/easier to read/easier to maintain code path. And maybe get a follow up to make it possible to maybe not invalidate everything in views_invalidate_cache but add a parameter to indicate if route rebuilding is needed. Something like that?

spokje’s picture

spokje
Primary language Dutch
commented
Issue tags: +Bug Smash Initiative

Surprised nobody have tagged this before now.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

At this time we will need a D10 MR also.

The MR 1956 also needs to be rerolled.

JeroenT opened merge request !3332

JeroenT closed merge request !1956

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Needs work » Needs review

I was struggling with rebasing 10.1.x. on the MR, so I created a new one.

smustgrave’s picture

smustgrave commented

Adding this to my list to review.

But yes it's usually easier to just make a separate MR for 10.1. Get all kinds of merge conflicts if I try to update a 9.5.x one.

smustgrave’s picture

smustgrave commented

Tested MR 3332

Not sure how to manually trigger but using the test case without the fix I did get a failure
Failed asserting that false is true.

Only nit was for the comment. WIll need in review if anyone doesn't think it's important.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Not sure how to manually trigger

The only way is to upload a test-only version of the MR as a patch at the moment.

A follow-up to make views_invalidate_cache() more precise sounds good, but I think this is fine as it is.

Committed/pushed to 10.1.x, thanks!

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Fixed » Reviewed & tested by the community

@catch,

I think something went wrong with the commit?

  • catch committed b55b81f5 on 10.1.x 
    Issue #3268872 by JeroenT, Spokje, smustgrave, alexpott, Lendude:...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Ooops, forgot to push I think. Should be there now!

Status: Fixed » Closed (fixed)

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