It is not possible to display multiple pagers on the same page as the pager element value is protected and can not passed to the theme_pager function (it will evaluate all pagers as element 0)

the only workaround is to use hard coded pager elements like

$query = db_select(..)->extend('PagerDefault')->element(42);
$build['mypager'] = array('#theme' => 'pager', '#element' => 42);

but the real solution would be to use the element from the query as it is already unique

$query = db_select(..)->extend('PagerDefault');
$build['mypager'] = array('#theme' => 'pager', '#element' => $query->element);

it is even in the documentation:

* After running this query, access $this->element to get the element for this
* query.
*/
protected function ensureElement() {

CommentFileSizeAuthor
#26 702778-26.patch2.66 KBpoker10
#21 702778-21.patch660 bytespoker10
#1 protected_pager_element.patch509 bytespasqualle

Issue fork drupal-702778

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

Comments

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented
Status: Active » Needs review
StatusFileSize
new protected_pager_element.patch509 bytes
ff1’s picture

ff1 commented

So... what does this patch actually do?

mattyoung’s picture

mattyoung commented

.

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented

@ff1: you can access and use $query->element. Display the pager associated to given $query..

ff1’s picture

ff1 commented

Ah... Simple! Thanks Pasqualle.

Seems like a no-brainer, especially as its already documented. My only problem is I don't really know how to test this. What's a simple way to introduce multiple pagers on the same page?

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented

simple example:

$query1 = db_select('node', 'n')
  ->fields('n', array('title'))
  ->extend('PagerDefault')
  ->limit(1);
$nodes = $query1->execute();

$rows = array();
foreach ($nodes as $node) {
  $rows[] = array(array('data' => $node->title));
}

$build['table1'] = array(
  '#theme' => 'table',
  '#rows' => $rows,
);
$build['pager1'] = array('#theme' => 'pager', '#element' => $query1->element);
//var_dump($query1->element);

$query2 = db_select('node', 'n')
  ->fields('n', array('title'))
  ->extend('PagerDefault')
  ->limit(1);
$nodes = $query2->execute();

$rows = array();
foreach ($nodes as $node) {
  $rows[] = array(array('data' => $node->title));
}

$build['table2'] = array(
  '#theme' => 'table',
  '#rows' => $rows,
);
$build['pager2'] = array('#theme' => 'pager', '#element' => $query2->element);
//var_dump($query2->element);

print render($build);
sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Makes sense.

I guess there should be some API documentation somewhere in code that needs to be updated?

bojanz’s picture

bojanz commented

Yes, definitely!

While we're at it, could we unprotect $maxElement too?
I did it in #885014: Add pager and tablesort to EntityFieldQuery since an alternative pager implementation needs to modify it in order not to conflict with the existing pager implementation on the same pager, however I'm not sure that #885014: Add pager and tablesort to EntityFieldQuery will land in D7...

That all together gives us much more flexibility in working with pagers...

crifi’s picture

crifi commented
Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

Bump it to the highest version. This needs work because of new dev version and git.

crifi’s picture

crifi commented

Is this already fixed? See #903110: Multiple pager support is partially broken.

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented
Issue tags: +Needs backport to D7

Fixing tags per http://drupal.org/node/1517250

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented

The element is still protected in D7. Not sure where it is in D8..

dawehner’s picture

dawehner
Primary language German
commented
Component: base system » database system
Issue summary: View changes

.

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 commented
Version: 8.2.x-dev » 7.x-dev

We have closed #2572533: Add tests for multiple pagers on a given page. So for D8 this is fixed. Moving this to D7.

Pasqualle opened merge request !446

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented
Status: Needs work » Needs review
pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented
poker10’s picture

poker10 commented

In D9 a new method was added to access the $element property, see: #1202484: Improve usage of 'pager.manager' service in PagerSelectExtender, allow code to know the pager element ID used. 

public function getElement(): int {
  $this->ensureElement();
  return $this->element;
}

Wouldn't it be a better approach than changing the property access modifier?

poker10’s picture

poker10 commented
Related issues: +#1202484: Improve usage of 'pager.manager' service in PagerSelectExtender, allow code to know the pager element ID used
StatusFileSize
new 702778-21.patch660 bytes

Something like this. But let @mcdruid decide what approach will be better for D7.

mcdruid’s picture

mcdruid commented
Issue tags: +Needs framework manager review

Hmm this is a little tricky.

The code comment that mentions accessing the element is on a protected function \PagerDefault::ensureElement():

   * After running this method, access $this->element to get the element for this
   * query.
   */
  protected function ensureElement() {
    if (!isset($this->element)) {
      $this->element = self::$maxElement++;
    }
  }

I think adding an accessor method per #20/21 is the "correct" way to do this (and would be a correct backport from D8/9), but the simple patch in #1 is more than 12 years old and any sites that have applied that patch would need to change their theme code to use a new accessor instead.

Is there any reason we couldn't do both; the correct and the pragmatic change combined?

I'll ask for Fabianx's opinion as Framework Manager, but I'd be happy to commit both patches (#1 and #21).

mcdruid’s picture

mcdruid commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs framework manager review +RTBM, +Needs change record

Fabianx's framework manager review:

It's not a good idea to make a protected property public - especially as we cannot guarantee its existence.

I think sites that use a patch can continue to use that patch (as it should still apply) and then can choose to use the new method or not.

Needs a Change Record and some Release notes but my vote is for #21 only.

I agree with that, so #21's RTBM.

mcdruid credited Fabianx.

mcdruid’s picture

mcdruid commented

Adding credit for the review.

poker10’s picture

poker10 commented
Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs backport to D7, -Needs change record
StatusFileSize
new 702778-26.patch2.66 KB

Drafted a change record here: https://www.drupal.org/node/3307777 (some parts taken from the D8 CR).

I was thinking that it would be good to also add tests from D8, so we can actually verify that the new function is working and returning correct ID. See D8 commit: https://git.drupalcode.org/project/drupal/-/commit/96dc01cf37f3a4570540bbd6ec6eff09c47d29a9

I have updated the patch - only checks with getElement() function are added. Reuploading the patch to verify. Please re-review, so we can commit this.

mcdruid’s picture

mcdruid commented

#3307795: Drupal 7.x core tests dissappeared for the current D7 drupalci testbot problem.

In the meantime, I ran D7 tests for 702778-26.patch on a local drupalci with the default PHP 7.4 and MySQL 5.7 and all tests passed.

I'll review the patch manually shortly and put it back to RTBC unless I spot any problems.

mcdruid’s picture

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

This looks good, and is low risk as it's just adding the accessor (plus tests, which is great)!

  • mcdruid committed fb78f2d on 7.x 
    Issue #702778 by Pasqualle, poker10, mcdruid, Fabianx: Multiple pagers...
mcdruid’s picture

mcdruid commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -RTBM

Thanks everybody; great to close an issue with a 6-digit nid as Fixed!

Status: Fixed » Closed (fixed)

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