Problem/Motivation

The taxonomy_tid Views argument default plugin doesn't set any cache tags. And it extends ArgumentDefaultPluginBase, which also doesn't set any cache tags.

But clearly if the plugin is configured to use the node being viewed, the cache tags of said node should be returned by the plugin.

Steps to reproduce

In most cases this won't be noticed because the node_list cache tag tends to be on the View. But using a module like views_custom_cache_tag, I could put a Views block on a Page node that only serves Article nodes and only has the `node_list:article` cache tag. Then if I edit the Page and change the terms that it references and reload the Page, I would see that the View block does not update.

Proposed resolution

Add getCacheTags() method to Drupal\taxonomy\Plugin\views\argument_default\Tid

Remaining tasks

User interface changes

nope

API changes

nope

Data model changes

nope

Release notes snippet

nope

CommentFileSizeAuthor
#4 3427374_4.patch1.31 KBvighneshh
#3 has_taxonomy_term_id.png74.51 KBdanflanagan8

Issue fork drupal-3427374

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

danflanagan8 created an issue. See original summary.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

Here's a View I built out to help reproduce that follows what's in the IS. I did a Standard install and added field_tags to Page content type. Then I put this block in the content region of the block layout. Indeed when I view a Page and then update its tags, the View block does not update to reflect that change.

uuid: 1c258cad-110b-467f-aed3-d89212011bde
langcode: en
status: true
dependencies:
  config:
    - field.storage.node.field_tags
    - node.type.article
  module:
    - node
    - taxonomy
    - user
    - views_custom_cache_tag
id: cache_demo
label: 'Cache Demo'
module: views
description: ''
tag: ''
base_table: node_field_data
base_field: nid
display:
  default:
    id: default
    display_title: Default
    display_plugin: default
    position: 0
    display_options:
      title: 'Cache Demo'
      fields:
        title:
          id: title
          table: node_field_data
          field: title
          relationship: none
          group_type: group
          admin_label: ''
          plugin_id: field
          label: ''
          exclude: false
          alter:
            alter_text: false
            text: ''
            make_link: false
            path: ''
            absolute: false
            external: false
            replace_spaces: false
            path_case: none
            trim_whitespace: false
            alt: ''
            rel: ''
            link_class: ''
            prefix: ''
            suffix: ''
            target: ''
            nl2br: false
            max_length: 0
            word_boundary: true
            ellipsis: true
            more_link: false
            more_link_text: ''
            more_link_path: ''
            strip_tags: false
            trim: false
            preserve_tags: ''
            html: false
          element_type: ''
          element_class: ''
          element_label_type: ''
          element_label_class: ''
          element_label_colon: true
          element_wrapper_type: ''
          element_wrapper_class: ''
          element_default_classes: true
          empty: ''
          hide_empty: false
          empty_zero: false
          hide_alter_empty: true
          click_sort_column: value
          type: string
          settings:
            link_to_entity: true
          group_column: value
          group_columns: {  }
          group_rows: true
          delta_limit: 0
          delta_offset: 0
          delta_reversed: false
          delta_first_last: false
          multi_type: separator
          separator: ', '
          field_api_classes: false
        field_tags:
          id: field_tags
          table: node__field_tags
          field: field_tags
          relationship: none
          group_type: group
          admin_label: ''
          plugin_id: field
          label: ''
          exclude: false
          alter:
            alter_text: false
            text: ''
            make_link: false
            path: ''
            absolute: false
            external: false
            replace_spaces: false
            path_case: none
            trim_whitespace: false
            alt: ''
            rel: ''
            link_class: ''
            prefix: ''
            suffix: ''
            target: ''
            nl2br: false
            max_length: 0
            word_boundary: true
            ellipsis: true
            more_link: false
            more_link_text: ''
            more_link_path: ''
            strip_tags: false
            trim: false
            preserve_tags: ''
            html: false
          element_type: ''
          element_class: ''
          element_label_type: ''
          element_label_class: ''
          element_label_colon: false
          element_wrapper_type: ''
          element_wrapper_class: ''
          element_default_classes: true
          empty: ''
          hide_empty: false
          empty_zero: false
          hide_alter_empty: true
          click_sort_column: target_id
          type: entity_reference_label
          settings:
            link: false
          group_column: target_id
          group_columns: {  }
          group_rows: true
          delta_limit: 0
          delta_offset: 0
          delta_reversed: false
          delta_first_last: false
          multi_type: separator
          separator: ', '
          field_api_classes: false
      pager:
        type: some
        options:
          offset: 0
          items_per_page: 5
      exposed_form:
        type: basic
        options:
          submit_button: Apply
          reset_button: false
          reset_button_label: Reset
          exposed_sorts_label: 'Sort by'
          expose_sort_order: true
          sort_asc_label: Asc
          sort_desc_label: Desc
      access:
        type: perm
        options:
          perm: 'access content'
      cache:
        type: custom_tag
        options:
          custom_tag: 'node_list:article'
      empty: {  }
      sorts:
        created:
          id: created
          table: node_field_data
          field: created
          relationship: none
          group_type: group
          admin_label: ''
          entity_type: node
          entity_field: created
          plugin_id: date
          order: DESC
          expose:
            label: ''
            field_identifier: ''
          exposed: false
          granularity: second
      arguments:
        tid:
          id: tid
          table: taxonomy_index
          field: tid
          relationship: none
          group_type: group
          admin_label: ''
          plugin_id: taxonomy_index_tid
          default_action: default
          exception:
            value: all
            title_enable: false
            title: All
          title_enable: false
          title: ''
          default_argument_type: taxonomy_tid
          default_argument_options:
            term_page: '1'
            node: true
            limit: false
            vids: {  }
            anyall: ','
          default_argument_skip_url: false
          summary_options:
            base_path: ''
            count: true
            override: false
            items_per_page: 25
          summary:
            sort_order: asc
            number_of_records: 0
            format: default_summary
          specify_validation: false
          validate:
            type: none
            fail: 'not found'
          validate_options: {  }
          break_phrase: false
          add_table: false
          require_value: false
          reduce_duplicates: false
      filters:
        status:
          id: status
          table: node_field_data
          field: status
          entity_type: node
          entity_field: status
          plugin_id: boolean
          value: '1'
          group: 1
          expose:
            operator: ''
        type:
          id: type
          table: node_field_data
          field: type
          entity_type: node
          entity_field: type
          plugin_id: bundle
          value:
            article: article
      style:
        type: default
      row:
        type: fields
      query:
        type: views_query
        options:
          query_comment: ''
          disable_sql_rewrite: false
          distinct: false
          replica: false
          query_tags: {  }
      relationships: {  }
      header: {  }
      footer: {  }
      display_extenders: {  }
    cache_metadata:
      max-age: -1
      contexts:
        - 'languages:language_content'
        - 'languages:language_interface'
        - url
        - 'user.node_grants:view'
        - user.permissions
      tags:
        - 'config:field.storage.node.field_tags'
  block_1:
    id: block_1
    display_title: Block
    display_plugin: block
    position: 1
    display_options:
      display_extenders: {  }
    cache_metadata:
      max-age: -1
      contexts:
        - 'languages:language_content'
        - 'languages:language_interface'
        - url
        - 'user.node_grants:view'
        - user.permissions
      tags:
        - 'config:field.storage.node.field_tags'
danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Issue summary: View changes
StatusFileSize
new has_taxonomy_term_id.png74.51 KB
vighneshh’s picture

vighneshh commented
Status: Active » Needs review
StatusFileSize
new 3427374_4.patch1.31 KB

I have added a patch that would invalidate the cache and add cache tags according to the taxonomy ID.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Issue tags: +Needs tests

@Vighneshh I don't understand why the terms would need their cache tags added. What's a scenario where those cache tags would be relevant?

Also adding the Needs test tag. I'm hoping we can just add some assertions to existing test cases in `Drupal\Tests\taxonomy\Kernel\Views\TaxonomyDefaultArgumentTest`.

danflanagan8 opened merge request !7930

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs work » Needs review
Issue tags: -Needs tests

I put in an MR with a fix and some tests.

I've thought a bit about my claim in #6 that we don't need the cache tags of any of the terms. We don't access any fields on any of the terms that might be returned, so the only relevant aspect of the terms that might change is the publish status. Of course, publish status would be handled by the validator plugin, not the argument_default plugin. All in all, I'm pretty confident we don't want to use the terms' cache tags.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Can the MR be updated for 11.x please

danflanagan8 closed merge request !7930

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Version: 10.3.x-dev » 11.0.x-dev
danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Version: 11.0.x-dev » 11.x-dev

danflanagan8 opened merge request !7941

danflanagan8 changed the visibility of the branch 3427374-argument-default-cache-tags to hidden.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Issue tags: +Needs Review Queue Initiative

Test-only job was ran already https://git.drupalcode.org/issue/drupal-3427374/-/jobs/1534215 won't post the output as it's large.

But looking at the solution this would only work for nodes right? What about other entity types?

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

> But looking at the solution this would only work for nodes right? What about other entity types?

The feature itself only exists for nodes, and the option text is clear about that:

Load default filter from node page, that's good for related taxonomy blocks

So that's all the caching we have to worry about too.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Oh didn't see that.

As mentioned in the summary hard to test this manually but believe the tests show the issue. So believe adding should be fine.

I don't believe this will need a CR but I've been wrong before.

catch’s picture

catch
he/him
Primary language English
commented

It might be worth a follow-up to make this work for any entity type, we could add an 'entity' option and deprecate the node one probably. MR looks fine but I'm short for time and didn't review enough to actually commit.

  • alexpott committed e4a77089 on 10.3.x 
    Issue #3427374 by danflanagan8, Vighneshh: taxonomy_tid...

  • alexpott committed 99ed4332 on 10.4.x 
    Issue #3427374 by danflanagan8, Vighneshh: taxonomy_tid...

  • alexpott committed 10be6458 on 11.0.x 
    Issue #3427374 by danflanagan8, Vighneshh: taxonomy_tid...

  • alexpott committed 6d3cdb77 on 11.x 
    Issue #3427374 by danflanagan8, Vighneshh: taxonomy_tid...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Sure we can open an issue to make this option not limited to just node but that does not feel like a specific follow-up of this issue.

Committed and pushed 6d3cdb7701 to 11.x and 10be645825 to 11.0.x and 99ed4332d1 to 10.4.x and e4a770899b to 10.3.x. Thanks!

I was wondering about upgrade paths because cache tags appear in views config - but these cache tags sure don't because they are way more dynamic.

Status: Fixed » Closed (fixed)

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

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

It tuns out this fix doesn't really do anything because Views do not calculate cacheability of argument_default plugins at runtime. D'oh!

The same goes for argument and argument_validator plugins as well. The fun will continue in #3091671: Validate user has access option in contextual filter does not bubble access cacheability for views page

ericgsmith’s picture

ericgsmith commented

Should this be reverted - or open a new issue to revert? As mentioned above - this does nothing and the issue still persists. Cache tags returned by the handler are added to the view when the view is saved, not at runtime.

#3091671: Validate user has access option in contextual filter does not bubble access cacheability for views page looks related but not general enough that it would solve this issue. I will open a new issue in a couple days unless the etiquette is to reopen this