Problem/Motivation

From #2501905-2: \Drupal\views\Plugin\views\argument_default\QueryParameter should specify a more specific cache context:

  public function getCacheContexts() {
    $contexts = [];
    // By definition arguments depends on the URL.
    // @todo Once contexts are properly injected into block views we could pull
    //   the information from there.
    $contexts[] = 'url';

Currently, adding an argument to a view makes the view cacheable per url even if the argument value depends only on a single query argument value.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#42 2514784-42.patch7.38 KBmalcolm_p
#41 2514784-41.patch7.4 KBmalcolm_p
#39 2514784-39.patch3.08 KBmalcolm_p
#33 2514784-33.patch2.33 KBmalcolm_p
#24 2514784-23.patch2.7 KBmparker17
#17 2514784-17.patch2.19 KBjibran
#12 2514784-10.patch3.68 KBjibran
#12 interdiff.txt1017 bytesjibran
#10 interdiff.txt737 bytesolli
#10 2514784-9.patch3.41 KBolli
#7 2514784-7.patch2.69 KBolli
#1 2514784-1.patch775 bytesolli
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

olli’s picture

olli CreditAttribution: olli commented
Status: Active » Needs review
FileSize
2514784-1.patch775 bytes
olli’s picture

olli CreditAttribution: olli commented
Related issues: +#2502151: Convert shortcut block to view
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Yeah right, ideally for blocks this is not true, but for now, this is the best thing we can do.

Do you mind adding a test coverage for that?

Status: Needs review » Needs work

The last submitted patch, 1: 2514784-1.patch, failed testing.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii commented
Issue tags: +Novice, +Needs manual testing, +Needs tests

The test failures seems to be related to changed cache contexts. We should also remove the comment and @todo. Also manual testing would be good.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +novice-php
olli’s picture

olli CreditAttribution: olli commented
Status: Needs work » Needs review
FileSize
2514784-7.patch2.69 KB

- Fixed test failures
- Removed the comment and @todo

Manually tested
1. Installed standard with latest head
2. Created article with a tag
3. Visited taxonomy/term/1 and checked that X-Drupal-Cache-Contexts header included 'url'
4. Re-saved taxonomy_term view
5. Visited taxonomy/term/1 and checked that X-Drupal-Cache-Contexts header didn't include 'url' but 'url.query_args.pagers:0'

Do we also need a hook_update_N() here that saves all views?

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented

I think so yeah. Not all though just the one using arguments.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Assigned: Unassigned » jibran
Issue tags: -Novice, -novice-php

Going to write an update hook.

olli’s picture

olli CreditAttribution: olli commented
FileSize
2514784-9.patch3.41 KB
interdiff.txt737 bytes

Here's views_update_8001(). I think we also need to update existing views using arguments.

olli’s picture

olli CreditAttribution: olli commented

#9: Great! I hope #10 helps.. Manually tested that and it seemed to work.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Assigned: jibran » Unassigned
Issue tags: -Needs manual testing
FileSize
interdiff.txt1017 bytes
2514784-10.patch3.68 KB

Wow nice x-post. Needs update hook test now. interdiff is against 7.

olli’s picture

olli CreditAttribution: olli commented 
+++ b/core/modules/views/views.install
@@ -11,3 +11,32 @@
+    ->condition('status', TRUE)

Why only enabled?

#12 seems to work but I got "mb_strtolower() expects parameter 1 to be string, array given Unicode.php:331" with drush updb.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs review » Needs work

Oh yeah we can remove the enable check. Nice catch.

Maybe mb_strtolower() error is because the value of display.*.display_options.arguments.* is an array not string.

Feel free to work on this. You can copy the \Drupal\system\Tests\Update\UpdateSchemaTest to test the hook. If you want to carry on with your patch that's fine by me as well.

The last submitted patch, 12: 2514784-10.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Issue tags: +D8 upgrade path
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs work » Needs review
Issue tags: -Needs tests, -D8 upgrade path
FileSize
2514784-17.patch2.19 KB

I recreated the patch. If we'll get this in before #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface then we don't need an update hook. There are is a test fix so we already have tests for this. No interdiff because I re-created it.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented

Why would we not need an update hook if we get this in first? Views config entities in HEAD already have cacheability metadata stored/exported.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Why would we not need an update hook if we get this in first? Views config entities in HEAD already have cacheability metadata stored/exported.

Well, we would have to resave the cache contexts of the view, so its reflected before rendering for the query.

olli’s picture

olli CreditAttribution: olli commented
Title: \Drupal\views\Plugin\views\argument\ArgumentPluginBase should specify a more specific cache context » \Drupal\views\Plugin\views\argument\ArgumentPluginBase should allow subplugins to specify a more specific url cache context

mgifford queued 17: 2514784-17.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 17: 2514784-17.patch, failed testing.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-4
CreditAttribution: mparker17 at Northern Commerce commented
Status: Needs work » Needs review

Re-rolled the patch in #17. There was a merge conflict in core/modules/views/src/Tests/Plugin/DisplayPageWebTest.php:

<<<<<<< changed in HEAD
    $this->assertCacheContexts(['languages:language_interface', 'route', 'theme', 'url']);
=======
    $this->assertCacheContexts(['languages:language_interface', 'theme', 'url.query_args']);
>>>>>>> patch from #17

... which I resolved to...

    $this->assertCacheContexts(['languages:language_interface', 'route', 'theme', 'url', 'url.query_args']);
mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-4
CreditAttribution: mparker17 at Northern Commerce commented
FileSize
2514784-23.patch2.7 KB

I should probably upload that re-rolled patch...

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Puh, this could break existing code out there which maybe relies on it?

Status: Needs review » Needs work

The last submitted patch, 24: 2514784-23.patch, failed testing.

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.

The last submitted patch, 24: 2514784-23.patch, failed testing.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

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

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

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

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

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

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

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

malcolm_p’s picture

malcolm_p CreditAttribution: malcolm_p commented
FileSize
2514784-33.patch2.33 KB

I've rerolled this for 8.5.x, were there any outstanding issues blocking this and the linked QueryParameter patch from making it into core? It seems like all of the argument related views plugins are now setting their own cache contexts.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs work » Needs review

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

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

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Related issues: +#3092715: Views relying on contextual filter defaults always get the url cache context

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

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

malcolm_p’s picture

malcolm_p CreditAttribution: malcolm_p commented
FileSize
2514784-39.patch3.08 KB

Re-roll for 8.9.x.

Status: Needs review » Needs work

The last submitted patch, 39: 2514784-39.patch, failed testing. View results

malcolm_p’s picture

malcolm_p CreditAttribution: malcolm_p commented
FileSize
2514784-41.patch7.4 KB

One more patch with updated configs forDrupal\KernelTests\Config\DefaultConfigTest::testModuleConfig and adding a hook_post_update based on #12.

malcolm_p’s picture

malcolm_p CreditAttribution: malcolm_p commented
FileSize
2514784-42.patch7.38 KB

And once more to fix the deprecation in the post_update hook.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

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

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

DieterHolvoet’s picture

DieterHolvoet CreditAttribution: DieterHolvoet at Minsky commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Last patch seems for 8.9?