Problem/Motivation
#1909474: Use light (lite) pager by default changed the default pager to use the mini pager, which doesn't have to do an expensive count() query.
However, most existing default views, like the frontpage, admin/content and so on still use the normal pager.
Proposed resolution
Change them to use the mini pager and update the exported default views.
Remaining tasks
User interface changes
API changes
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | pager-test-full-nl.png | 30.63 KB | anavarre |
| #13 | pager-test-full.png | 62.16 KB | anavarre |
| #13 | pager-test.png | 70.27 KB | anavarre |
| #11 | interdiff-8-11.txt | 1.38 KB | anavarre |
| #11 | 2469157-11.patch | 8.06 KB | anavarre |
Comments
Comment #1
dawehnerSome tags ...
Comment #2
anavarreIn its most basic form, I *think* this is what we want to do here. But if all views are now using the mini pager, should tests continue to test for the full pager?
Comment #4
berdirWe should have tests anyway for different pagers, I doubt those are using those views.
Yes, that's the basic change, but you need to make additional changes to the configuration of those as they have less/different options.
Comment #5
olli commentedI think we could also just drop the pager in feed displays (like the frontpage rss.xml and #2375773: Remove pager from default taxonomy term feed). Not sure about changing admin/* views to use the mini pager.
Comment #6
berdirI'm pretty sure :)
I have 550k nodes, the normal pager executes a query that takes 1.5s+ (because of the join on the user table), by using the mini pager by default, the page is twice as fast.
Removing the pager completely from aggregator_rss_feed sounds like a good idea and just display a fixed amount of entries.
Comment #7
dawehnerWe have both a mini pager and a normal full pager test, so as long we don't replace the usage of those:
'test_store_pager_settings', 'test_pager_none', 'test_pager_some', 'test_pager_full', 'test_view_pager_full_zero_items_per_page',we should be fine.
I'm also fine to not switch over all test views in general, it doesn't give us any big advantage to do it.
Note: There is a RFC about paged feeds, see http://tools.ietf.org/html/rfc5005 but we don't provide support for that
For now we maybe should replace the pager from a limit of items per page.
Comment #8
anavarreSo, here's hopefully a better version to change from full to mini pager. No interdiff provided because there's nothing to really compare against.
I haven't yet addressed changes that haven't been confirmed for sure, such as no pager for aggregator_rss_feed or even replacing the mini pager by a fixed number of items.
Comment #9
anavarreComment #11
anavarreMaking the tests pass by removing lines for the full pagers? Doesn't seem right to me :-/
What are we supposed to do here?
Comment #12
berdirStrange, can we maybe switch that test to use one of the test views?
Comment #13
anavarre@berdir: I'm sorry but I'm not sure I parsed what you said correctly or know what to do.
What I see is that we have PagerTest.php and MiniPagerTest.php and what is failing in PagerTest.php is that we no longer have a full pager to test against. This is why removing the corresponding lines as in #11 made the tests pass. See Simpletest verbose message screenshot below:
I did try to add the below line to add back the full pager only in the realm of the test:
$display['display_options']['pager']['type'] = 'full';But this keeps throwing failed tests while we now have a full pager:
Note that the pager links, while being displayed in english, work fine when we remove the full pager lines. Odd since we're defining a 'nl' langcode.
In testPagerLocale() we do set the same langcode and the pager terms in NL are displayed/detected just fine.
Comment #14
dawehnerSo what I think we should do is to not replace that test, but rather have a test not using the default content view, but instead have a dedicated test for full pager view with configuration translation and a dedicated test for mini pager with config translation, so basically swap out the usage of the content view to custom test views.
Does that make sense?
Comment #15
jp.stacey commentedThis still seems to be in the specification stage and I don't think it's currently suitable for novices. I'm therefore untagging it for now.
If the issue can be clearly scoped, and the issue summary updated with tasks, it could be suitable for novices again!
Comment #16
jp.stacey commented(Triaged as part of Sprint Weekend 2016.)
Comment #25
dkmishra commentedI have tried to apply patch #11 but it failed on drupal version 8.9.3.