Problem/Motivation

From #3170185: Drupal\taxonomy\Form\OverviewTerms should determine the current page via the 'pager.manager' service, not from the Request:

given the example code in \Drupal\Core\Pager\PagerManagerInterface::createPager() it'd be nice to implement PagerManagerInterface::findPage() that calls through to the pager parameters service.

Actually the example code referes to a non-existing getCurrentPage() method.

Steps to reproduce

Proposed resolution

Implement a PagerManagerInterface::findPage() method. Wait for D10 not to break interface BC in D9.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#17 interdiff-16.txt1.11 KBPooja Ganjage
#16 3173636-16.patch3.43 KBPooja Ganjage
#14 interdiff-13.txt1.09 KBPooja Ganjage
#13 3173636-13.patch3.43 KBPooja Ganjage
#9 3173636-8.patch3.7 KBPooja Ganjage
#8 interdiff_5_8.txt893 bytesanmolgoyal74
#8 3173636-8.patch3.43 KBanmolgoyal74
#6 interdiff.txt1.13 KBpaulocs
#5 3173636-5.patch2.76 KBpaulocs
#3 3173636-3.patch1.87 KBpaulocs
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and commented
Version: 10.0.x-dev » 9.1.x-dev

I think we can do this under the 1-to-1 interface rule. Plus there are no classes implementing this in contrib according to http://grep.xnddx.ru/search?text=PagerManagerInterface&filename=&page=1

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
Status: Active » Needs review
FileSize
3173636-3.patch1.87 KB

A patch for it.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and commented
Status: Needs review » Needs work
Issue tags: +Needs tests
paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3173636-5.patch2.76 KB

I added the tests.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
FileSize
interdiff.txt1.13 KB

Interdiff.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and commented
Status: Needs review » Needs work

We need to update the documentation. What's fascinating is that the docs already point to a PagerManager::findPage() method...

   *   // Perform the query, using the requested offset from
   *   // PagerManagerInterface::findPage(). This comes from a URL parameter, so
   *   // here we are assuming that the URL parameter corresponds to an actual
   *   // page of results that will exist within the set.
   *   $pager_parameters = \Drupal::service('pager.parameters');
   *   $page = $pager_parameters->findPage();

Anyhow this pseudo-code should be updated to use the pager.manager service.

anmolgoyal74’s picture

anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commented
Status: Needs work » Needs review
FileSize
3173636-8.patch3.43 KB
interdiff_5_8.txt893 bytes

Updated the documentation as pointed out in #7

Pooja Ganjage’s picture

Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commented
FileSize
3173636-8.patch3.7 KB

Hi,

I am creating patch as per the given suggestion by #7 comments.

Please review the patch.

Thanks.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs review » Reviewed & tested by the community

#8 is RTBC for me.

#9 has no interdiff and a change too much to docs IMHO.

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.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/lib/Drupal/Core/Pager/PagerManagerInterface.php
@@ -120,6 +120,24 @@ public function createPager($total, $limit, $element = 0);
+   */
+  public function findPage($pager_id = 0);
+

I think we should type and return hint int with the new method.

Pooja Ganjage’s picture

Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commented
FileSize
3173636-13.patch3.43 KB

Hi,

Creating a patch as per the #12 comment suggestion.

Please review the patch.

Thanks.

Pooja Ganjage’s picture

Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commented
FileSize
interdiff-13.txt1.09 KB

Attached interdiff for #13 patch.

mondrake’s picture

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

Let’s add an int typehint to the $pager_id parameter too, then

Pooja Ganjage’s picture

Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commented
FileSize
3173636-16.patch3.43 KB

Updated patch as per the #15 comment suggestion.

Pooja Ganjage’s picture

Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commented
FileSize
interdiff-16.txt1.11 KB

Attached interdiff for #16 patch.

Pooja Ganjage’s picture

Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commented
Status: Needs work » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs review » Reviewed & tested by the community

looks ok now thanks

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Issue tags: +Needs change record, +Needs release note

Thank you! Just needs a change record and release note snippet now.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
Issue tags: -Needs change record

I just create a change record. Can you please review it?
Not sure if it is enough.

catch’s picture

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

The CR could use a before/after example ideally - could be based on the change that'll be made in #3170185: Drupal\taxonomy\Form\OverviewTerms should determine the current page via the 'pager.manager' service, not from the Request.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented

Hi Catch,

I just add the Before/After example to the CR.

  • catch committed a5789f3 on 9.2.x 
    Issue #3173636 by Pooja Ganjage, paulocs, anmolgoyal74, mondrake,...
catch’s picture

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

Committed a5789f3 and pushed to 9.2.x. Thanks!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Issue tags: -Needs release note

Thank you. The CR is enough here, don't think we need a release note, so untagging.

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

Published the CR