Problem/Motivation

All pagers get a unique element ID by default (in execute()), but this ID is never revealed to the user, so it cannot be used when displaying the pager with

$page[] = array(
  '#theme' => 'pager',
  '#element' => $pagerid, // <= unknown!
);

However it is nice to allow users to set the element ID with the element($id) function, but as they might never know which tables and pagers are displayed at once, it is not very safe to use numbers 1, 2, 3, etc. (as it is not possible to use string names as ID).

Proposed resolution

I suggest to enrich the PagerDefault class with this public function:

/**
 * Return element ID for this pager query. If not set yet, it sets it.
 */
public function getElement() {
  $this->ensureElement();
  return $this->element;
}

This will make it possible to have automatically "named" pagers and display them correctly. And I don't think the element($id) function will be used anymore.

In case the expected pager usage (mainly displaying the right pager) is different as I think and this function is not needed, please explain, how to display two pagers without setting arbitrary element id number by element($id) function. (Thanks)

Remaining tasks

Commit

User interface changes

None

API changes

  • The new method Drupal\Core\Database\Query\PagerSelectExtender::getElement() which returns the element ID for the pager query.
  • The new method Drupal\Core\Database\Connection::getPagerManager() which returns the pager manager service.
  • The method Drupal\Core\Pager\PagerManager::getMaxPagerElementId() has been made public (was protected).
  • The new method Drupal\Core\Pager\PagerManager::reservePagerElementId() which reserves a pager element ID.

Data model changes

None

Release notes snippet

TBD

CommentFileSizeAuthor
#13 1202484-13-do-not-test.patch10.86 KBmondrake
#13 1202484-13.patch42.54 KBmondrake
#8 d8-identify-pagers-1202484-8.patch678 bytesRajendar Reddy
#4 d7-identify-pagers-1202484-0.patch519 bytesMichael Zetterberg fd. Lopez
#4 d8-identify-pagers-1202484-0.patch682 bytesMichael Zetterberg fd. Lopez

Issue fork drupal-1202484

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcingy’s picture

marcingy CreditAttribution: marcingy commented
Version: 7.x-dev » 8.x-dev

Features are commited in latest version first.

c4rl’s picture

c4rl CreditAttribution: c4rl commented

Subscribe. We should really use string names. Seems blatantly obvious.

kingandy’s picture

kingandy CreditAttribution: kingandy commented

String names would make it lengthier to pass multiple pager states back and forth (at the moment it's a simple "page=0,0,1,0,3" in the query strings), but yes, much easier to keep track of as a developer.

Michael Zetterberg fd. Lopez’s picture

Michael Zetterberg fd. Lopez CreditAttribution: Michael Zetterberg fd. Lopez commented
FileSize
d8-identify-pagers-1202484-0.patch682 bytes
d7-identify-pagers-1202484-0.patch519 bytes

I like the getter approach better than just making the element property public like in #702778: Multiple pagers on the same page.

You will still need access to the pager instance to use this feature request, which you do not have sometimes for various reasons. What I do right now to solve my problem (even though I have access to the pager instance) is to read PagerDefault::$maxElement. The value returned from the method in this feature request is the same as PagerDefault::$maxElement - 1 right after invoking execute() on the pager.

I have attached 2 patches, one for D7 and one for D8. Maybe that will help move this issue towards solved.

Michael Zetterberg fd. Lopez’s picture

Michael Zetterberg fd. Lopez CreditAttribution: Michael Zetterberg fd. Lopez commented
Status: Active » Needs review

Forgot the status...

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn commented
Issue summary: View changes
Status: Needs review » Needs work

There's some whitespace issues.

In these lines.

+++ b/core/lib/Drupal/Core/Database/Query/PagerSelectExtender.php
@@ -169,4 +169,17 @@ class PagerSelectExtender extends SelectExtender {
+  ¶
+  /**
+   * Return the element ID for this pager query.
+   * ¶
+   * If not set yet, it sets it.
+   * ¶
marcingy’s picture

marcingy CreditAttribution: marcingy commented

4: d8-identify-pagers-1202484-0.patch queued for re-testing.

Rajendar Reddy’s picture

Rajendar Reddy CreditAttribution: Rajendar Reddy commented
Status: Needs work » Needs review
FileSize
d8-identify-pagers-1202484-8.patch678 bytes

Updating patch with reroll. Please review.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue tags: +Needs tests

I guess we need some tests

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

Patch still applies.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs review » Needs work

I'd say ensureElement() should return the element, and then getElement() would just call it. Or really just change the name of ensureElement() to getElement() and make it public.

Either way it's probably an API change, so it might not make it in during RC.

The patch still applies, but it still needs tests.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Component: base system » database system

Well, its an API addition, but yeah I don't see any reason why this could/should not be able to land in 8.0.x

Moving to the right component ...

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs work » Needs review
Related issues: +#2044435: Convert pager.inc to a service
FileSize
1202484-13.patch42.54 KB
1202484-13-do-not-test.patch10.86 KB

It's a bit far off, but I ended up building this on top of #2044435: Convert pager.inc to a service, as I was trying to understand implications of converting the pager to a service to the PagerSelectExtender. As such this is for 8.1.x or later.

However I think the tests could be adjusted to work with the patch in #8 if anyone wants to give a try.

Do not test patch is on top of #2044435: Convert pager.inc to a service, full patch for the bot. No interdiff as it won't make so much sense.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Title: Pager's element ID not known by the user of the class! » Code can not know the pager element ID used by the database system unless it is set explicitly

Updated title.

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.

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.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Postponed

Postponed on #2044435: Convert pager.inc to a service.

daffie’s picture

daffie CreditAttribution: daffie commented
Version: 8.2.x-dev » 8.3.x-dev
Issue tags: -Needs tests

I think that the tests in SelectPagerDefaultTest with the extra tests being added is enough testing.

This issue relies on #2044435: Convert pager.inc to a service. And that issue is going to be fixed on Drupal 8.3.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Title: Code can not know the pager element ID used by the database system unless it is set explicitly » [PP-2] Code can not know the pager element ID used by the database system unless it is set explicitly

@daffie I think we would need an intermediate step after #2044435: Convert pager.inc to a service before coming to this one: Refactor Database/Query/PagerSelectExtender to use the new service.

I haven't opened an issue for that yet, waiting for the milestone of #2044435: Convert pager.inc to a service before opening its follow-ups.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Parent issue: » #2547833: Pager.inc -- add tests, clean it up, convert to a service, use it!
Related issues: +#2801285: [PP-1] Refactor PagerSelectExtender to use the pager.manager service

Opened #2801285: [PP-1] Refactor PagerSelectExtender to use the pager.manager service.

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now 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.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now 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.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now 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.

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

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now 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.

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

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.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.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Title: [PP-2] Code can not know the pager element ID used by the database system unless it is set explicitly » Code can not know the pager element ID used by the database system unless it is set explicitly
Status: Postponed » Active

#2044435: Convert pager.inc to a service is in! This can be unblocked now.

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

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now 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.

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

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

mondrake opened merge request !103

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Active » Needs review
Issue tags: +Needs tests

Moved issue to Merge Request workflow, patch is practically a new one, reroll of the one in #13 wouldn't make sense.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Title: Code can not know the pager element ID used by the database system unless it is set explicitly » Improve usage of 'pager.manager' service in PagerSelectExtender, allow code to know the pager element ID used
daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work

Patch looks good.

Added a couple of comments on the MR.

raman.b made their first commit to this issue’s fork.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented

Added CR https://www.drupal.org/node/3194594

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs work » Needs review
Issue tags: -Needs tests
daffie’s picture

daffie CreditAttribution: daffie commented

Asking for your opinion @mondrake.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented

Commented in MR

daffie’s picture

daffie CreditAttribution: daffie commented

Replied on the MR.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented

Let's see what bot thinks.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work

Testbot fails.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs work » Needs review
daffie’s picture

daffie CreditAttribution: daffie commented
Issue summary: View changes
daffie’s picture

daffie CreditAttribution: daffie commented

Updated the IS and the CR.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented

Comment in MR

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Reviewed & tested by the community

All the code changes look good to me.
The IS and CR are in order.
All the new methods have testing.
For me it is RTBC.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs work » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs review » Needs work
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

We can probably deprecate without a trigger_error() in this case - we do that for global constants and similar too.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented

Alright, but in this case we should have a BC shim to keep the property storing the real value, do we?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Yes that's still needed.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Assigned: Unassigned » mondrake

on this

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs work » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Assigned: mondrake » Unassigned
daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work

The MR is failing the testbot.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented

Um, latest changes show that the patch here is subtly flawed: $maxElement in HEAD now is increased on calling element(), while in the patch here the pager ID is only increased in execute() when the Pager object is created. This opens the door to bugs in case code opens multiple selects without executing them - they would all get the same pager id.

Can’t create a Pager object at the element() call, either, since we won’t have the needed args for that yet.

IMHO we need a kind of reservePagerId method on the PagerManager to take into account this, and probably manage the max pager Id state in a property there.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs work » Needs review

Done #57

daffie’s picture

daffie CreditAttribution: daffie commented
Issue summary: View changes

Updated the IS and the CR with the new method Drupal\Core\Pager\PagerManager::reservePagerElementId().

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work

Moving it back to needs work for the nitpick.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs work » Needs review

Fixed the nit and optimized a bit ensureElement.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Reviewed & tested by the community

All code changes look good to me.
All new methods have testing.
The public static variable is a good as it gets deprecated.
It is all documented in the IS and the CR.
For me it is RTBC.

  • catch committed 96dc01c on 9.2.x 
    Issue #1202484 by mondrake, raman.b, Michael Zetterberg fd. Lopez,...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Thanks that looks better with the bc layer.

Committed 96dc01c and pushed to 9.2.x. Thanks!

mondrake closed merge request !103

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented

This caused a small issue in contrib, which IMHO should be solved in a core test module: #3202014: pager_test_preprocess_pager() should return if pager element is not defined

Status: Fixed » Closed (fixed)

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