Problem/Motivation
#3427374: taxonomy_tid ViewsArgumentDefault plugin doesn't add cache tags was committed in attempt to fix an issue where when "Load default filter from node page, that's good for related taxonomy blocks" is used the view does not include any cache metadata from the node being viewed, resulting in stale data displayed after updates.
As mentioned in https://www.drupal.org/project/drupal/issues/3427374#comment-15645693 this fix does nothing and the issue persists. Opening this as a new issue given the time that has passed since that was closed.
Cache metadata is collected from the handlers when the view is saved (https://git.drupalcode.org/project/drupal/-/blob/11.2.8/core/modules/vie...) - cache metadata is not collected at runtime from the argument / handler.
#3091671: Validate user has access option in contextual filter does not bubble access cacheability for views page is a similar issue - where cache metadata from the argument validator doesn't bubble up.
Together - both issues suggest we need a better way to apply cache metadata at run time from various views plugins. Or a clarified process to do this properly.
From that related issue @danflanagan8 identified an existing issue #2853592: Cacheability metadata can't be set from within argument default handlers. and test / test plugin in core that implies a supported way to do this is to apply cache data to the view element itself.
Steps to reproduce
Using the standard profile:
- Create a new view block display of taxonomy terms, add a contextual filter for "Taxonomy term: Term ID"
- Select "Provide default value" for "When the filter value is NOT available"
- Select "Taxonomy term ID from URL"
- Select "Load default filter from node page, that's good for related taxonomy blocks"
- Place the block for article content type
- Add an article with tags, observe the view is present
- Edit the article, observe the view is present with the old data
Proposed resolution
Head in same direction as #3091671: Validate user has access option in contextual filter does not bubble access cacheability for views page - apply the cache metadata to the view element itself when the default value is loaded - unless there is a better way to do this.
Remaining tasks
- Revert 3427374
- Add a test using an actual view, test was flawed in 3427374 so we potentially need to verify via a functional test here?
- Fix cache metadata
- Review
User interface changes
n/a
Introduced terminology
n/a
API changes
n/a
Data model changes
n/a
Release notes snippet
| Comment | File | Size | Author |
|---|
Issue fork drupal-3560357
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:
- 3560357-add-break-phrase
changes, plain diff MR !15038
- 3560357-taxonomy-term-id
changes, plain diff MR !13975
Comments
Comment #3
ericgsmith commentedhttps://git.drupalcode.org/issue/drupal-3560357/-/jobs/7465803#L57 test only change shows the issue.
After adding the test I see there is already an existing
TaxonomyDefaultArgumentTestjust testing a view title - I guess we should move this view and test into that existing test rather than a new one. Happy to do that, will wait for a feedback on the change and test itself first.Comment #4
ericgsmith commentedComment #6
smustgrave commentedThis one has been around for a while
I did a rebase and everything is still all green.
Test-only run is still failing as expected https://git.drupalcode.org/issue/drupal-3560357/-/jobs/8138898
Not 100% sure if a CR is needed to remove getCacheTags() from the plugin but change of adding CacheableMetadata makes sense to me.
No additional feedback.
Comment #8
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #9
smustgrave commentedBot rebellion
Comment #10
alexpottCommitted and pushed 8ebac7b2e26 to main and 6c383e892bb to 11.x and 8df2a7d84e6 to 11.3.x. Thanks!
Comment #17
godotislateThis broke HEAD for SQLite and Postgres tests.
Issue is that there are two taxonomy terms referenced by the node, so the argument default plugin returns
1+2, but the test view was not configured to handle multiple results. Updated the view config in MR 15038 and ran the SQLite and Postrgres tests whichpassed:
https://git.drupalcode.org/project/drupal/-/pipelines/764387
https://git.drupalcode.org/project/drupal/-/pipelines/764388
Comment #19
alexpottLet's just commit @godotislate's new MR - nice fix. I'm not a huge fan of the break phrase stuff and we have to wonder how this view works on MySQL when it is doing a where
tid = "1+2"Comment #24
catchCommitted/pushed the follow-up to main, 11.x and 11.3.x, thanks!