Problem/Motivation

Any time a pager is used, the cache context for that pager should bubble up and force different cache items (variants) to be created. If that doesn't happen, any pager for which no cache context is defined will appear broken with #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!).

Proposed resolution

Blocked on #2429257: Bubble cache contexts and #2430341: Comment pager-dependent cache key should be a cache context.

It makes sense for this to happen automatically for any pager. That is possible once #2429257: Bubble cache contexts is in.

Then we can make the pager (see pager.inc) automatically set #cache['contexts']. Note though that the pager is currently rendered via the theme system (#theme => pager); we probably want to change that to use the render system. That'd be consistent with the rest of the system in D8. We'd need a Pager element class (so we can use #type => pager instead), which has a #pre_render callback for setting #cache['contexts'].

Final note: the first pager cache context is being manually added in #2430341: Comment pager-dependent cache key should be a cache context, because that example doesn't need cache context bubbling to work. However, in this issue, we should aim to delete comment_entity_build_defaults_alter() altogether, because it should become obsolete.

Remaining tasks

  1. Discuss
  2. Do it!

User interface changes

None.

API changes

#theme => pager must become #type => pager

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because pagers depend on a cache context, but don't associate one. We don't want developers to have to associate pagers manually, so we do it automatically whenever a pager is used.
Issue priority Major because without the appropriate cache context, cache poisoning will happen.
Prioritized changes The main goal of this issue is performance.
Disruption Disruptive for contributed and custom modules in a very minor way: it requires them to switch from #theme => pager to #type => pager.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Title: [PP-2] Ensure every pager automatically gets » [PP-2] Ensure every pager automatically associates a matching cache context
Wim Leers’s picture

Title: [PP-2] Ensure every pager automatically associates a matching cache context » [PP-1] Ensure every pager automatically associates a matching cache context
mondrake’s picture

Wim Leers’s picture

Title: [PP-1] Ensure every pager automatically associates a matching cache context » Ensure every pager automatically associates a matching cache context
Status: Postponed » Active

#2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance landed, this is now unblocked.


#3: good question. It might be related, but I don't think it is, since we never ever cache AJAX responses (yet) in Drupal.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

Working on this one today.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Active » Needs review
FileSize
14.56 KB

#2430341: Comment pager-dependent cache key should be a cache context introduced the pager cache context and used it for one specific usage of the pager.

Now that we have cache context bubbling, we can convert #theme => pager to #type => pager and automatically add the necessary cache context.

Wim Leers’s picture

FileSize
15.13 KB
613 bytes

I thought I wouldn't be able to handle the custom Views one (views_mini_pager) also, but turns out Views doesn't do anything special, it just reuses the regular pager entirely. That means that all of Views' pagers are just in fact using the standard pager system, so no special handling is necessary, which means I can close #2433591: Views using pagers should specify a cache context . Hurray!

dawehner’s picture

Well, we use the standard rendering m

+++ b/core/lib/Drupal/Core/Render/ElementInfoManager.php
@@ -57,6 +57,9 @@ public function getInfo($type) {
+    if (!is_string($type)) {
+      var_dump($type);exit;
+    }

Debug code

+++ b/core/modules/views/src/Plugin/views/pager/Full.php
@@ -91,6 +91,7 @@ public function render($input) {
+      '#type' => 'pager',
       '#theme' => $this->themeFunctions(),

+++ b/core/modules/views/src/Plugin/views/pager/Mini.php
@@ -99,6 +99,7 @@ public function render($input) {
+      '#type' => 'pager',
       '#theme' => $this->themeFunctions(),

So this basically uses type pager but custom theme functions? Nice.

Fabianx’s picture

Yes, this is how the theme system was devised:

#type sets default values, which can be e.g. theme, but are added via += so already set values take precedent.

Fabianx’s picture

Status: Needs review » Needs work

RTBC - if there was no debug code ;), hence CNW.

Might need also a test? Not sure.

dawehner’s picture

Well, for sure we need some tests.

On top of that, I think its not that easy, necessarily

Views doesn't use the DBTNG pager implementation but provides its own, for that it uses simple PagerPluginBase::getCurrentPage()
Possible sources for that:

  • The URL
  • An exposed form
  • Panels content-type / block configuration
  • Arbitrary API calls to $view->setCurrentPage()

Given that I'm not convinced that just fetching the cache context always from the URL

  public function getContext($pager_id) {
    return 'pager.' . $pager_id . '.' . pager_find_page($pager_id);
  }

is the perfect solution.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
14.47 KB
708 bytes

This reroll only fixes #8.1 (lol, oops!), tests are indeed still needed.


#11: right, that's exactly what I feared to find in Views' pagers. :(

But I don't fully understand it currently. Please help me.

  • The URL: no problem there, that's how core's pagers work.
  • Exposed form: how can an exposed form affect the pager, other than pointing you to a different URL? (An AJAX-paged View surely is the same as pointing to a different URL?)
  • Panels content type/block configuration: I have no idea at all what this means. A block can be configured to never show page 1? Or?
  • Arbitrary API calls: this is unacceptablevery non-ideal, it breaks cacheability. And AFAICT it's actually intended to be a private API: it's only used in tests, and internally, to e.g. unserialize Views results cache items into a ViewExecutable object. So… not actually a problem?
dawehner’s picture

Arbitrary API calls: this is unacceptable, it breaks cacheability. And AFAICT it's actually intended to be a private API: it's only used in tests, and internally, to e.g. unserialize Views results cache items into a ViewExecutable object. So… not actually a problem?

I'm actively not commenting this tonight.

Wim Leers’s picture

I apologize for the harsh way I described bullet 4: I said unacceptable, by which I really meant very non-ideal. I used the word "unacceptable" because that's a word we've seen in the past when discussing things that are breaking cacheability.
I've updated my comment with <del> and <ins> now.


I had a brief chat with dawehner in IRC to clear things up, and below is my take on that.


I did say AFAICT it's actually intended to be a private API: […], and that "AFAICT" of course means please correct me if I'm wrong.

It's totally fine to override pager settings (the pager ID, the number of items per page, and so on), but those things need to happen sufficiently early. And that actually happens, both in D8 core and in D7 contrib (e.g. http://cgit.drupalcode.org/ctools/tree/views_content/plugins/content_typ...).

The only thing I find very problematic is claiming that we must be able to call setCurrentPage() at any time during the request. AFAICT, in core it is only called for tests and internally. I can't find any D7 contrib modules calling the D7 equivalent at all.
But even then, calling setCurrentPage() could be fine as long as the code doing that also ensures the view render array also gets a #cache[context] set for the contexts in which that happens.

So I don't think we actually disagree as much as you thought? :)

But I do think there is no reason we want setCurrentPage() to be a public API. Unless anybody can think of a convincing use case.


  • Regarding "panels content type/block configuration": I understand that now — that's only about a pane/block being able to override the pager configuration. That's totally fine, because it's deterministic.
  • Regarding exposed forms: I still don't get that. Could you explain that?
  • Views doesn't use the DBTNG pager implementation but provides its own […] +
        return 'pager.' . $pager_id . '.' . pager_find_page($pager_id);
    

    — now I understand. However, looking at SqlBase::setCurrentPage(), it is actually using exactly the same logic as the regular pager: it uses $request->query->get('page') to find the values for the other pagers, and then updates the global $pager_page_array with the page number it is asked to set. Which AFAICT means it's fine to use pager_find_page()?

Wim Leers’s picture

Issue tags: -Needs tests
FileSize
16.56 KB
2.73 KB

Test added.

Fabianx’s picture

I think this is fine, probably views should be changed to use pager_get_page() instead so that that code is synchronized and making setCurrentPage a private API should be good.

Anything that changes globally is giving a 'promise' in this Drupal system that it can't hold.

Unless it makes it _always_ and sets the page and everything to uncacheable, but I don't think that is something we want.

Fabianx’s picture

Assigned: Unassigned » dawehner
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs change record

I checked the PR again and it is simple enough. For me this is RTBC, though needs a change record for the '#type => pager' usage, CNW for that.

It is too bad thedavidmeisters works to never use #theme (unless to override) and always use #type did not get into core in time.

I leave this to dawehner (assigning for that case) if he is okay with the cache context calling pager_get_page() and views using a similar but compatible version or if we should split this up again.

This would also profit from a beta-evaluation.

Fabianx’s picture

Status: Reviewed & tested by the community » Needs work
Wim Leers’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record +API change

Added beta evaluation & CR.

Back to RTBC per #17, still assigned to dawehner per #17.

damiankloip’s picture

I don't see how just making stuff private that was not before is a good option here? People use view objects quite a lot e.g. to process to the results from directly. So we just remove a part of the API? So if I want to just set a page offset for a view, I now have to jump through all the global pager hoops? I see where you are trying to head, but that seems like quite a big regression in terms on DX?

Fabianx’s picture

Status: Reviewed & tested by the community » Needs work

#20 that is right.

So the example would be:

A block that has some custom code that is displaying something always in the same way, but sets a pager, then we don't want a pager cache context on that, because in the worst case the pager would react suddenly to the main page pager.

I think we need to split this off back to the views issue, then discuss in detail. (Sorry, Wim)

dawehner’s picture

Assigned: dawehner » Unassigned

Thank you for taking other people's opinion into account!!! I experienced other things lately

Fabianx’s picture

Title: Ensure every pager automatically associates a matching cache context » Ensure every (non-views) pager automatically associates a matching cache context

- Re-classifying title
- I also thought again about the #theme => pager conversion thing and I think the exception you proposed in IRC (provided we know the original render array - not sure) is warranted.

Let's get catch' and others opinion on that:

What is worse:

- an exception that breaks BC?
- not getting an associated page context and e.g. having to rely on the more granular cache_context.url / route set elsewhere (or having to set that yourselves)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
15.42 KB
1.17 KB

Removed the Views hunks. Turns out I should've followed my initial hunch to not touch Views because it'd be a special case as usual.

Wim Leers’s picture

FileSize
17.61 KB
2.47 KB

The exception I proposed in IRC as mentioned in #23, now implemented. Give it a try by modifying #type => pager to #theme => pager and posting a single comment.

The two choices are:

  1. allow developers to still use #theme => pager, and hence allow for cache poisoning, which breaks sites and potentially causes information disclosure. This puts the burden of setting #cache['contexts'] manually whenever they use #theme => pager. Whenever they forget: cache poisoning/information disclosure.
  2. require #type => pager, and throw an exception to help developers get it right

Status: Needs review » Needs work

The last submitted patch, 25: pager-2433599-25.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
18.74 KB
1.17 KB

#25 also requires the #24 interdiff to be reverted, otherwise no View that uses a pager can be rendered without triggering this same exception. That explains the 592 failures.

Fabianx’s picture

Assigned: Unassigned » catch

Maybe we need a #views_pager type then for now, which does not trigger that Exception (if we want the exception).

Ugh ...

Assigning to catch for a decision on what route we want to go :).

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

I am RTBCing this for now.

The views cache context is only right for simple cases, but we have a follow-up that is specificially dedicated to views and the worst that can happen is that a block is cached per page that should not be, which is better than cache poisoning and can be fixed by altering the view render array even in contrib.

So its a minor / normal bug, which is not blocking, while views is more complicated and we want to get this right.

Note to core committers:

This throws a new Exception when #theme => pager is used, but the #type => pager will ensure the cache context is set correctly, where currently the cache would be wrong.

Wim Leers’s picture

To clarify: the #27 interdiff shows that we're updating Views' pagers to use #type => pager. We have to do that because otherwise the exception added in #25 (to force developers to use #type => pager instead of #theme => pager) actually triggers an exception for every rendered view :)

This is less than ideal, because as @dawehner pointed out, Views supports much more advanced pagers, that depend basically on any possible logic, and not just the query parameter-based pager. So that's why we have #2433591: Views using pagers should specify a cache context : to sort out those advanced use cases in Views. So this patch may lie for some Views (complex/advanced ones), but it's actually correct for the simple ones core ships with.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 27: pager-2433599-27.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
18.74 KB

Straight reroll.

Wim Leers’s picture

FileSize
17.36 KB

catch pinged me in IRC:

catch: WimLeers: so on pagers, if we add the exception it should be in a follow-up
catch: WimLeers: and I don't think pager cache poisoning is a big deal.
WimLeers: catch: ok
catch: Views pagers didn't work for lots of reasons in 7.x for a long time.

That means reverting to #24, without reverting the docs fixes in #25.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

Wim Leers’s picture

Great, now we can continue in #2433591: Views using pagers should specify a cache context . Also published the CR.

  • catch committed 8f5fa6a on 8.0.x
    Issue #2433599 by Wim Leers: Ensure every (non-views) pager...
catch’s picture

This may have caused a random failure in HEAD:

FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,553 pass(es), 2 fail(s), and 0 exception(s).

Individual Environment Summaries
==============================================

-- [[SimpleTest]]: [PHP 5.4 MySQL] --

* Drupal\node\Tests\NodeListBuilderTest (2 pass(es), 1 fail(s), and 0 exception(s))
   - [fail] [Other] "Value array (
  0 => 'node_view_grants',
) is equal to value array (
  0 => 'node_view_grants',
  1 => 'pager:0',
)." in NodeListBuilderTest.php on line 40 of Drupal\node\Tests\NodeListBuilderTest->testCacheContexts().

* Drupal\system\Tests\Entity\EntityListBuilderTest (27 pass(es), 1 fail(s), and 0 exception(s))
   - [fail] [Other] "Value array (
  0 => 'entity_test_view_grants',
) is equal to value array (
  0 => 'entity_test_view_grants',
  1 => 'pager:0',
)." in EntityListBuilderTest.php on line 69 of Drupal\system\Tests\Entity\EntityListBuilderTest->testCacheContexts().
alexpott’s picture

Nope it wasn't a random fail. It was a commit conflict of sorts with #2445743: Allow views base tables and entity types to define additional cache contexts

Status: Fixed » Closed (fixed)

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