Problem/Motivation
pager.inc uses global variables, has confusing/incomplete documentation, and is lacking in automated tests.
This issue aims to track the various efforts in play to make this code better. Some of these issues are pretty stale -- I've included them so we can see discussions that have already happened.
- Cleanup and move docs from pager.inc to the new classes
- Custom/contrib code cannot access the pager element ID used by the database system unless it is set explicitly: #1202484: Improve usage of 'pager.manager' service in PagerSelectExtender, allow code to know the pager element ID used
- Issues around zero-based query-string arguments: #1818040: Pager should start counting from 1, not 0, #385270: Make the GET page variable consistent with the real page number, #1024226: Option to start Pager count at 1 instead of 0,
Proposed resolution
Convert Drupal's default pager to a service. This makes it easy to replace -- the zero-based vs. one-based debate could be solved right there!
Unfortunately proper cleanup would include removing the four globals in pager.inc. Since that's an API change, the best we can do in 8.x is deprecate them and include (deprecated) wrappers for the new service to maintain backwards comparability until 9.x.
Coming soon.
Tasks done
- Global variables (eeww!): #259740: Stop using global variables in theme_pager()
- Use of globals in preprocess functions: #2229983: Remove globals on template_preprocess_pager
- Refactor core/modules/system/tests/modules/pager_test/src/Controller/PagerTestController to use the new service
- Refactor Entity/Query/QueryBase to use the new service
- Refactor Cache/Context/PagersCacheContext to use the new service
- views/pager/SqlBase manipulates directly
$pager_*global variables - The taxonomy OverviewTerms form manipulates directly
$pager_*global variables: #2770649: [PP-1] Refactor OverviewTerms form to use the pager.factory service - Refactor Database/Query/PagerSelectExtender to use the new service: #2801285: [PP-1] Refactor PagerSelectExtender to use the pager.manager service
- core/modules/views/views.theme.inc manipulates directly
$pager_*global variables - Conversion to a service: #2044435: Convert pager.inc to a service
- #2572533: Add tests for multiple pagers on a given page
- #2539472: Pager "first" and "previous" links have incorrect URLs
- #2530296: Fix up docs in core/includes/pager.inc
Remaining tasks
All of them...
User interface changes
None.
API changes
None -- include wrappers to maintain backwards compatibility.
Data model changes
None.
Comments
Comment #2
larowlanThere is an issue to convert it to a service that was green at one stage
Comment #3
mikeker commented#2: Found it. Thanks, @larowlan, I've added it to the issue summary along with the proposal to convert pagers into a service.
Comment #4
mikeker commentedComment #5
mondrakeComment #6
mondrakeRevived #2044435: Convert pager.inc to a service, and removed from issue summary the @todo code snippet that was removed in #2539472: Pager "first" and "previous" links have incorrect URLs.
Comment #7
mondrakeComment #8
mondrakeComment #9
mondrakeMoved #2530296: Fix up docs in core/includes/pager.inc to tasks done, updated IS.
Comment #10
mondrakeComment #12
mondrakeComment #13
mondrake#2572533: Add tests for multiple pagers on a given page has been committed.
Comment #14
mondrakeComment #15
mondrakeComment #17
mondrakeComment #18
dawehnerA little bit nicer title :)
Comment #19
mondrakeComment #20
mondrakeComment #21
mondrakeComment #22
mondrakeComment #23
mondrakeComment #28
mondrakeComment #29
mile23I've done a little refactoring on #2044435: Convert pager.inc to a service which seems to be the blocker here.
I'm switching the parent of #2044435: Convert pager.inc to a service from this issue to #2999721: [META] Deprecate the legacy include files before Drupal 9 because deprecating include files seems to have more momentum than just re-working the pager for its own sake.
Comment #32
mondrakeComment #33
kim.pepper#2044435: Convert pager.inc to a service is in, so should we mark this Fixed?
Comment #34
mondrakeI think so, since the remaining issues in the IS are probably better to live on their own.
Fixed, and a thought goes to @mikeker.