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
- Discuss
- Do it!
User interface changes
None.
API changes
#theme => pager
must become #type => pager
Beta phase evaluation
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 . |
Comment | File | Size | Author |
---|---|---|---|
#33 | pager-2433599-33.patch | 17.36 KB | Wim Leers |
Comments
Comment #1
Wim LeersComment #2
Wim Leers#2430341: Comment pager-dependent cache key should be a cache context landed.
Comment #3
mondrakeJust wondering if this could be of any help to #2182555: On Ajax-enabled forms with a pager, pager is disappearing on form validation error and on Ajax form reload? Sorry if this is off topic.
Comment #4
Wim Leers#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.
Comment #5
Wim LeersWorking on this one today.
Comment #6
Wim Leers#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.Comment #7
Wim LeersI 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!Comment #8
dawehnerWell, we use the standard rendering m
Debug code
So this basically uses type pager but custom theme functions? Nice.
Comment #9
Fabianx CreditAttribution: Fabianx commentedYes, 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.
Comment #10
Fabianx CreditAttribution: Fabianx commentedRTBC - if there was no debug code ;), hence CNW.
Might need also a test? Not sure.
Comment #11
dawehnerWell, 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:
$view->setCurrentPage()
Given that I'm not convinced that just fetching the cache context always from the URL
is the perfect solution.
Comment #12
Wim LeersThis 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.
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 aViewExecutable
object. So… not actually a problem?Comment #13
dawehnerI'm actively not commenting this tonight.
Comment #14
Wim LeersI apologize for the harsh way I described bullet 4: I said
, by which I really meant . 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
, and that "AFAICT" of course means .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.— 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 theglobal $pager_page_array
with the page number it is asked to set. Which AFAICT means it's fine to usepager_find_page()
?Comment #15
Wim LeersTest added.
Comment #16
Fabianx CreditAttribution: Fabianx commentedI 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.
Comment #17
Fabianx CreditAttribution: Fabianx commentedI 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.
Comment #18
Fabianx CreditAttribution: Fabianx commentedComment #19
Wim LeersAdded beta evaluation & CR.
Back to RTBC per #17, still assigned to dawehner per #17.
Comment #20
damiankloip CreditAttribution: damiankloip commentedI 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?
Comment #21
Fabianx CreditAttribution: Fabianx commented#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)
Comment #22
dawehnerThank you for taking other people's opinion into account!!! I experienced other things lately
Comment #23
Fabianx CreditAttribution: Fabianx commented- 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)
Comment #24
Wim LeersRemoved 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.
Comment #25
Wim LeersThe 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:
#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.#type => pager
, and throw an exception to help developers get it rightComment #27
Wim Leers#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.
Comment #28
Fabianx CreditAttribution: Fabianx commentedMaybe 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 :).
Comment #29
Fabianx CreditAttribution: Fabianx commentedI 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.
Comment #30
Wim LeersTo 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.
Comment #32
Wim LeersStraight reroll.
Comment #33
Wim Leerscatch pinged me in IRC:
That means reverting to #24, without reverting the docs fixes in #25.
Comment #34
catchCommitted/pushed to 8.0.x, thanks!
Comment #35
Wim LeersGreat, now we can continue in #2433591: Views using pagers should specify a cache context . Also published the CR.
Comment #37
catchThis may have caused a random failure in HEAD:
Comment #38
alexpottNope 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