Convert pager.inc to a service

Tagging

Firstly *awesome* more globals down.
A few questions/observations.

+++ b/core/lib/Drupal/Component/Pager/Pager.phpundefined
@@ -0,0 +1,310 @@
+   * Each key in the array is itself an associative array with the following
+   * properties:
+   *   'totalItems' - total number of items in the pager
+   *   'totalPages' - total number of pages in the pager
+   *   'limit' - items per page
+   *   'current' - current page
...
+    static::$elements[$element][$key] = $value;

Does this warrant a helper class? Having a first class object would make a stronger API thank an associative array.

+++ b/core/lib/Drupal/Component/Pager/Pager.phpundefined
@@ -0,0 +1,310 @@
+    $page = isset($_GET['page']) ? $_GET['page'] : '';

This should use Request but would that mean it's no longer drupal agnostic?

+++ b/core/lib/Drupal/Component/Pager/Pager.phpundefined
@@ -0,0 +1,310 @@
+    self::set('totalItems', $total, $element);

I think we prefer static over self, it allows for the class to be extended

+++ b/core/lib/Drupal/Component/Pager/Pager.phpundefined
@@ -0,0 +1,310 @@
+    $query = &drupal_static(__CLASS__ . '::' . __FUNCTION__);
...
+      $query = drupal_get_query_parameters($_GET, array('page'));

Drupal specific shouldn't be used in Drupal\Component

For the static can we just make a protected property?

+++ b/core/lib/Drupal/Component/Pager/Pager.phpundefined
@@ -0,0 +1,310 @@
+    $max_element = max(array_keys(static::$elements));
...
+      $element_pages[] = ($i == $element) ? $page : self::getCurrentPage($i);

Any reason for mixing self and static?

I am doubtful that this issue would be accepted at this point in the cycle. It's an API change, doesn't buy us much (just lazy loading; it doesn't fix that our pager system is AFU to begin with), and there's nothing critical or major blocked on it.

The verification process is here: https://drupal.org/node/2045345

Things in Component can be registered as a service. It would just have to live in Drupal\Core\Pager instead. A service just means "a read only object available via the container" (more or less).

Now, if we could simplify and less-fugly the pager without a hard API break, that's something to consider. Do we have options there to at least clean up the code?

Tagging to get some exposure, it might be allowable anyway given the main use for the pager (views) is in core anyway.

So, re-filing based on the current consensus on this issue. This is used two places in core: Views UI, and the demented hydra that is the taxonomy overview form (see #1974492: Convert taxonomy_overview_terms to a Form Controller). I'm actually sort of with Crell in that doing this this doesn't gain us this much immediately--at least, from the Taxonomy side, the form that uses this is not going to be cleanly decoupled any time soon anyway. I'll ping Views UI co-maintainers to see what they thing about the globals.

Changing the tests tag to the more universally used one.

Why not using https://github.com/whiteoctober/Pagerfanta ? I'm using it for symfony project and it's pretty simple and solid.

At least in the abstract I'd be fine with dropping our pager and using Pagerfanta.

@Crell
Pagerfanta does not even support our default pager which is the mini pager. That one cannot support the AdapterInterface they provide (see https://github.com/whiteoctober/Pagerfanta/issues/145)

We are on the brink of releasing RC1. You may want to check on IRC (#drupal-contribute) to see if this issue will be able to make it into Drupal 8 at all, before you spend time on this.

This looks like a good conversion to me so far... it does need tests :(

Just picked out a few ultra trivial nits while following along.
[mostly @file is deprecated ]

I have added the newline.

Thanks for the nudge I will have a look at

#2572533: Add tests for multiple pagers on a given page

sometime this weekend.

@mondrake I have only looked at the surface of that issue.. but between here and there there is a lot of work of yours that looks good and is unreviewed ... that is a shame. Is there anything else you want me to look at?

+interface PagerInterface extends ContainerInjectionInterface

Suppose this interface should define only pager related methods, so container injection is not needed

My first review for the pager service:

1. +++ b/core/core.services.yml
   @@ -33,6 +33,7 @@ parameters:
   +  pager_class: Drupal\Core\Pager\Pager
   

   I am not sure if this right or not, but I have noticed that there are no other classes defined like this. Can you explain.

2. +++ b/core/lib/Drupal/Core/Pager/PagerFactoryInterface.php
   @@ -0,0 +1,65 @@
   +  public function get($element);
   

   The name of this method is a bit vague. Not sure what would be better. Maybe something like getPager()?!

3. +++ b/core/lib/Drupal/Core/Pager/PagerFactoryInterface.php
   @@ -0,0 +1,65 @@
   +  public function all();
   

   The name of this method is a bit vague. Not sure what would be better. Maybe something like getAllPagers()?!

4. +++ b/core/lib/Drupal/Core/Pager/PagerInterface.php
   @@ -0,0 +1,196 @@
   +   * Gets the pager element.
   

   Can you add some more documentation what exactly is a pager element. Also in relation to the other parts of a pager.

5. +++ b/core/lib/Drupal/Core/Pager/PagerInterface.php
   @@ -0,0 +1,196 @@
   +  /**
   +   * Initializes the pager.
   +   *
   +   * @param int $total
   +   *   The total number of items to be paged.
   +   * @param int $limit
   +   *   The number of items the calling code will display per page.
   +   *
   +   * @return \Drupal\Core\Pager\PagerInterface
   +   *   The Pager object.
   +   */
   +  public function init($total, $limit);
   

   Maybe a stupid question, but why not use the create() method instead of init()?

6. +++ b/core/lib/Drupal/Core/Pager/PagerInterface.php
   @@ -0,0 +1,196 @@
   +   * @var $total_pages int
   ...
   +   * @var $total_items int
   ...
   +   * @var $limit int
   

   You do not define those variable in the interface , so what the interface is concerned they do not exist. They are just method parameters (@param).

7. +++ b/core/lib/Drupal/Core/Pager/PagerInterface.php
   @@ -0,0 +1,196 @@
   +  /**
   +   * Gets a pager link.
   +   *
   +   * @param int $page
   +   *   The target page.
   +   *
   +   * @return string
   +   *   The URL of the pager link.
   +   */
   +  public function getLink($page);
   

   Can you add some more documentation what exactly is a pager link. Also in relation to the other parts of a pager.

8. +++ b/core/lib/Drupal/Core/Pager/Pager.php
   @@ -0,0 +1,317 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function create(ContainerInterface $container, $element = NULL) {
   

   The Pager::create() method get it inheritance from ContainerInjectionInterface only you add an extra parameter to the method. Not sure if that is allowed. What is this method return value?

9. +++ b/core/lib/Drupal/Core/Pager/Pager.php
   @@ -0,0 +1,317 @@
   +    $this->currentPage = $page;
   ...
   +    $this->totalPages = $total_pages;
   ...
   +    $this->totalItems = $total_items;
   ...
   +    $this->limit = $limit;
   

   Can we add typecasting: $this->variable = (int) $variable.

10. Not instances of the deprecated functions in core/pager.inc are replaced by the new Pager service in the rest of core. Can you do that. Maybe we shall find some bugs we did not think of.

The file core/core.service.php had some additions, so small reroll is needed.

Good work mondrake. Lets work on this together and get this fixed.

My second review for the pager service:

1. +++ b/core/includes/pager.inc
   @@ -191,13 +191,13 @@ function template_preprocess_pager(&$variables) {
      // Calculate various markers within this pager piece:
      // Middle is used to "center" pages around the current page.
      $pager_middle = ceil($quantity / 2);
      // current is the page we are currently paged to.
   -  $pager_current = $pager_page_array[$element] + 1;
   +  $pager_current = $pager->getCurrentPage() + 1;
      // first is the first page listed by this pager piece (re quantity).
      $pager_first = $pager_current - $pager_middle + 1;
      // last is the last page listed by this pager piece (re quantity).
      $pager_last = $pager_current + $quantity - $pager_middle;
      // max is the maximum page number.
   -  $pager_max = $pager_total[$element];
   +  $pager_max = $pager->getTotalPages();
      // End of marker calculations.
   

   Minor problem with these function variables. I am fine with using these variables or with using the one you get from the pager object. But now we are mixing them in the rest of the function and that is confusing. Like $pager_current and $pager->getCurrentPage(). If we choose for using the pager object maybe we should add some extra method to the pager object.

2. I do not like the naming of the "element" variable. For me does not feel right and I find it confusing. I think that "id" would be a better name. I do not know if we can change it and maybe there is a very good reason why it is named as it is. On the other side, when do we get an other chance to change the name of this variable.

3. +++ b/core/lib/Drupal/Core/Pager/PagerInterface.php
   @@ -0,0 +1,208 @@
   +   * Gets the current page (0-index).
   ...
   +   * Sets the current page for this pager (0-index).
   ...
   +   * Gets last page in the pager (0-index).
   

   I have seen that there is an issue to change the pager from 0-indexed to 1-indexed. If we want to use the same PagerInterface for that, then we should remove the "0-index" parts from the PagerInterface.

4. +++ b/core/lib/Drupal/Core/Pager/PagerFactory.php
   @@ -0,0 +1,126 @@
   +  /**
   +   * The array of pager objects.
   +   *
   +   * @var \Drupal\Core\Pager\PagerInterface[]
   +   */
   +  protected $pagers = [];
   

   Not 100% sure, but I think that the pager objects are indexed by their element. Can you add that to the documentation.

5. +++ b/core/lib/Drupal/Core/Pager/PagerFactory.php
   @@ -0,0 +1,126 @@
   +  public function getFirstAvailableElementId() {
   

   Are the pagers indexed by their Element or by their ElementId? Or rename the method to getFirstAvailablePagerId?

6. +++ b/core/lib/Drupal/Core/Pager/PagerFactoryInterface.php
   @@ -0,0 +1,65 @@
   +  /**
   +   * Returns the current array of pager objects.
   +   *
   +   * @return \Drupal\Core\Pager\Pager[]
   +   *   The array of pager objects.
   +   */
   +  public function all();
   

   Array of pager object indexed by their element.

7. For comment #70.2 and #70.3: Good explanation lets leave it as it is.
8. For comment #70.5: The Pager::init() method is fine.
9. Can we do the change from comment #70.8 for the Pager::create() method.
10. For comment #70.10: Lets do it in the follow-up issues.
11. We need a change record for this issue. Can we add the part from comment #40 with how to change the pager class.

Todo: review the tests.

My review of the tests for the pager service:

1. +++ b/core/tests/Drupal/Tests/Core/Pager/PagerFactoryTest.php
   @@ -0,0 +1,128 @@
   +    // Pager element should be 0.
   

   Pager element should be 2.

2. +++ b/core/tests/Drupal/Tests/Core/Pager/PagerFactoryTest.php
   @@ -0,0 +1,128 @@
   +  public function testAll() {
   

   Can we also check that the elements are 4 and 6.

3. +++ b/core/tests/Drupal/Tests/Core/Pager/PagerFactoryTest.php
   @@ -0,0 +1,128 @@
   +    $this->assertSame(['foo' => 'bar', 'qux' => 'dee'], $this->pagerFactory->getCurrentRequestQueryParameters());
   ...
   +    $this->assertSame('bar', $this->pagerFactory->getCurrentRequestQueryParameter('foo'));
   +    $this->assertSame('dee', $this->pagerFactory->getCurrentRequestQueryParameter('qux'));
   ...
   +    $this->assertSame('', $this->pagerFactory->getCurrentRequestQueryParameter('zot'));
   

   These tests are not testing much. With the method getCurrentRequestQuery() being mocked.

I think the patch looks great now and almost ready for RTBC. I have one point left:

+++ b/core/tests/Drupal/Tests/Core/Pager/PagerFactoryTest.php
@@ -0,0 +1,130 @@
+    $this->assertSame(4, $pager->getElement());
...
+    $this->assertSame(6, $pager->getElement());

I think we had a miscommunication. The change I was looking for was $this->assertSame([4, 6], array_keys($this->pagerFactory->all()));. We are testing the method PagerFactory::all().

Thank you for explaining why we cannot rename the element part.

I have added a change record.

It all looks good to me. All my remarks are explained or addressed, so RTBC from me.

Good work mondrake!

Back to RTBC.

Sorry to move it back to needs work, but given we can improve method names and what not, I think its not the worst idea to move it to to needs work :(
It doesn't mean at all that the patch is wrong, but rather it needs some additional work.

1. +++ b/core/core.services.yml
   @@ -41,6 +41,9 @@ parameters:
   +    class: Drupal\Core\Pager\Pager
   

   It is a bit weird to make the class configurable, as we don't do it anywhere else.

2. +++ b/core/lib/Drupal/Core/Pager/Pager.php
   @@ -0,0 +1,317 @@
   +    // Gets the element's current page from the current request.
   +    $page = $this->getCurrentPage();
   

   This comment is also a bit not helpful. Maybe just skip it is not too bad.

3. +++ b/core/lib/Drupal/Core/Pager/Pager.php
   @@ -0,0 +1,317 @@
   +    // We calculate the total of pages as ceil(items / limit).
   ...
   +      ->setTotalPages((int) ceil($this->getTotalItems() / $limit))
   

   At least for me this comment doesn't really add value. It would be otherwise better to explain WHY the calculation is done in that way.

4. +++ b/core/lib/Drupal/Core/Pager/Pager.php
   @@ -0,0 +1,317 @@
   +  public function getLink($page) {
   +    $options = ['query' => $this->getLinkQueryParameters($page)];
   +    return Url::fromRoute($this->getRouteName(), $this->getRouteParameters(), $options);
   +  }
   

   This method is returning a URL, so it should be named getUrl() or maybe even toUrl() which seems to be a pattern we use more and more.

5. +++ b/core/lib/Drupal/Core/Pager/PagerFactoryInterface.php
   @@ -0,0 +1,76 @@
   +  /**
   +   * Returns the current array of pager objects.
   +   *
   +   * @return \Drupal\Core\Pager\Pager[]
   +   *   The array of pager objects, indexed by their element.
   +   */
   +  public function all();
   

   Can we clarify what current means on the interface? Is it every initialized one or is it every available in the current query parameter of the request?

6. +++ b/core/lib/Drupal/Core/Pager/PagerFactoryInterface.php
   @@ -0,0 +1,76 @@
   +  public function getQueryStringParameter();
   ...
   +  public function getCurrentRequestQueryParameter($parameter);
   

   It is a bit odd that we need both.

7. +++ b/core/lib/Drupal/Core/Pager/PagerInterface.php
   @@ -0,0 +1,225 @@
   +   * Gets the route name for this pager.
   +   *
   +   * @return string
   +   *   The route name.
   +   */
   +  public function getRouteName();
   +
   +  /**
   +   * Sets the route name for this pager.
   +   *
   +   * @param string $route_name
   +   *   The route name.
   +   *
   +   * @return \Drupal\Core\Pager\PagerInterface
   +   *   The Pager object.
   +   */
   +  public function setRouteName($route_name);
   +
   +  /**
   +   * Gets the route parameters for this pager.
   +   *
   +   * @return string[]
   +   *   The route parameters.
   +   */
   +  public function getRouteParameters();
   +
   +  /**
   +   * Sets the route parameters for this pager.
   +   *
   +   * @param string[] $route_parameters
   +   *   The route parameters.
   +   *
   +   * @return \Drupal\Core\Pager\PagerInterface
   +   *   The Pager object.
   +   */
   +  public function setRouteParameters(array $route_parameters);
   

   We could maybe replace all those methods with a method toUrl($route_name, $route_parameters, $options), which then also sets the query parameters of the page properly

8. +++ b/core/lib/Drupal/Core/Pager/PagerInterface.php
   @@ -0,0 +1,225 @@
   +   * @return \Drupal\Core\Url
   ...
   +  public function getLink($page);
   

   As written before, I think getLink is not the best name for it :)

9. +++ b/core/tests/Drupal/Tests/Core/Pager/PagerFactoryTest.php
   @@ -0,0 +1,138 @@
   +    // @todo drop globals in D9.
   +    global $pager_page_array, $pager_total, $pager_total_items, $pager_limits;
   +    $this->backupGlobals = [$pager_page_array, $pager_total, $pager_total_items, $pager_limits];
   +
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function tearDown() {
   +    // @todo drop globals in D9.
   +    global $pager_page_array, $pager_total, $pager_total_items, $pager_limits;
   +    list($pager_page_array, $pager_total, $pager_total_items, $pager_limits) = $this->backupGlobals;
   +  }
   

   Can't use use https://phpunit.de/manual/5.5/en/appendixes.annotations.html#appendixes.annotations.backupGlobals directly?

10. +++ b/core/tests/Drupal/Tests/Core/Pager/PagerFactoryTest.php
    @@ -0,0 +1,138 @@
    +    $this->assertSame(0, $this->pagerFactory->GetFirstAvailableElement());
    ...
    +    $this->assertSame(1, $this->pagerFactory->GetFirstAvailableElement());
    ...
    +    $this->assertSame(5, $this->pagerFactory->GetFirstAvailableElement());
    

    PHP is case insensitive, but still let's use getFirstAvailableElement()

1. See #73.1, #38, #40. My intention is to be able to swap the implementation of the Pager class without having to swap PagerFactory too. If there are better ways, let me know. I will surely use this in contrib.

Well, its a pattern we don't use anywhere else in core. A reason why I personally believe its not that useful is that if you replace a class, you also often need new dependencies. When this is the case, the container parameter doesn't help you.

9. I thought about that. but that would apply for any global. Since we know the exact names of the globals involved, I thought it is more accurate to make the test excluding those only. No problem to change that if you insist.

It just feels safer to let PHPUnit handle it directly, as they maybe more things, for example when a test fails ... one never knows.

Thank you @mondrake!

@mondrake
Sorry for causing that trouble :(

@mondrake: Can you tell me how you are going to override the Pager class in contrib?

@mondrake: Thank you for the explanation.

The patch is almost RTBC for me. I do have some minor documentation remarks:

1. +++ b/core/includes/pager.inc
   @@ -123,18 +123,15 @@ function pager_find_page($element = 0) {
   -  // We calculate the total of pages as ceil(items / limit).
   

   Can we change the test to be the same as with Pagar::init().

2. +++ b/core/lib/Drupal/Core/Pager/Pager.php
   @@ -0,0 +1,249 @@
   +    // page ($limit). We take the nearest integer to accomodate any remainder.
   

   Ceil() is not the nearest integer, the next integer by rounding up the calculated value.

3. +++ b/core/tests/Drupal/Tests/Core/Pager/PagerTest.php
   @@ -0,0 +1,129 @@
   + * @todo remove the backupGlobals directive once $pager_page_array,
   + *   $pager_total, $pager_total_items, $pager_limits globals have been removed.
   + *
   + * @backupGlobals disabled
   

   We are not using the pager globals in the PagetTest, so these comments are not nessecary.

@mondrake: For 110.1 you are right and my mistake. For 110.3, thank you for explaining.

The patch look great again.
All remarks of @dawehner are addressed. Thank you for your help @dawehner!

For me it is back to RTBC.

Great work @mondrake!

Testbot problems

+++ b/core/includes/pager.inc
@@ -173,105 +173,93 @@ function pager_get_query_parameters() {
-  // Calculate various markers within this pager piece:
+  // Calculate various markers within this pager list:
   // Middle is used to "center" pages around the current page.
-  $pager_middle = ceil($quantity / 2);
-  // current is the page we are currently paged to.
-  $pager_current = $pager_page_array[$element] + 1;
-  // first is the first page listed by this pager piece (re quantity).
-  $pager_first = $pager_current - $pager_middle + 1;
-  // last is the last page listed by this pager piece (re quantity).
-  $pager_last = $pager_current + $quantity - $pager_middle;
-  // max is the maximum page number.
-  $pager_max = $pager_total[$element];
+  $list_middle = ceil($quantity / 2);
+  // Current is the page we are currently paged to.
+  $list_current = $pager->getCurrentPage() + 1;
+  // First is the first page listed by this pager list (re quantity).
+  $list_first = $list_current - $list_middle + 1;
+  // Last is the last page listed by this pager list (re quantity).
+  $list_last = $list_current + $quantity - $list_middle;
   // End of marker calculations.
 
   // Prepare for generation loop.
-  $i = $pager_first;
-  if ($pager_last > $pager_max) {
+  $i = $list_first;
+  if ($list_last > $pager->getTotalPages()) {
     // Adjust "center" if at end of query.
-    $i = $i + ($pager_max - $pager_last);
-    $pager_last = $pager_max;
+    $i = $i + ($pager->getTotalPages() - $list_last);
+    $list_last = $pager->getTotalPages();
   }
   if ($i <= 0) {
     // Adjust "center" if at start of query.
-    $pager_last = $pager_last + (1 - $i);
+    $list_last = $list_last + (1 - $i);
     $i = 1;
   }
   // End of generation loop preparation.

This is really icky change. The change from pager_ to list_ doesn't help understanding at all. Is there no way that more of the logic here can be encapsulated in the class? In an ideal would the template_preprocess_pager would not exist or have the minimum logic possible.

Talked to @alexpott on IRC and wants the changes to core/includes/pager.inc changed back as he stated in comment #116. This patch does that.

@mondrake: You did not step on my toes. I should have looked better add the patch. Thanks for fixing it. Just happy that we got this issue moving again.

The changes that @alexpott wanted are done.

+++ b/core/lib/Drupal/Core/Pager/Pager.php
@@ -0,0 +1,250 @@
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container, $element) {
+    $instance = new static(
+      $container->get('pager.factory')
+    );
+
+    // Set the pager element.
+    $instance->element = $element;
+
+    // @todo: Start of BC layer. This maps the pager properties to the global
+    // pager variables of D8. When removing the global variables in D9, remove
+    // the following lines.
+    global $pager_page_array, $pager_total, $pager_total_items, $pager_limits;
+    if (!isset($pager_page_array[$element])) {
+      $pager_page_array[$element] = NULL;
+    }
+    $instance->currentPage = &$pager_page_array[$element];
+    if (!isset($pager_total[$element])) {
+      $pager_total[$element] = NULL;
+    }
+    $instance->totalPages = &$pager_total[$element];
+    if (!isset($pager_total_items[$element])) {
+      $pager_total_items[$element] = NULL;
+    }
+    $instance->totalItems = &$pager_total_items[$element];
+    if (!isset($pager_limits[$element])) {
+      $pager_limits[$element] = NULL;
+    }
+    $instance->limit = &$pager_limits[$element];
+    // @todo: End of BC layer.
+
+    return $instance;

+++ b/core/lib/Drupal/Core/Pager/PagerFactory.php
@@ -0,0 +1,138 @@
+    return Pager::create($this->container, $element);

This is odd... we're borrowing from the \Drupal\Core\DependencyInjection\ContainerInjectionInterface but we're not implementing it. Why is this not just all in the constructor? Seems unnecessarily complex and if someone did just used the constructor atm it wouldn't work. Especially since this method will be largely redundant once the bc layer is removed.

Looks good to me.
All wishes of @alexpott are implemented.
For me it is RTBC.

@alexpott: Thank you for reviewing.

Wrong status.

1. Sorry the reviews keep on coming in drips and drabs - I just keep on looking at this patch and feel uneasy about it and then find more things that concern me. This time it is the mutability of the pager objects once they've been constructed. It just feels wrong that they should be so changeable.

2. +++ b/core/lib/Drupal/Core/Pager/Pager.php
   @@ -0,0 +1,238 @@
   +      $pager_page_array[$element] = NULL;
   ...
   +      $pager_total[$element] = NULL;
   ...
   +      $pager_total_items[$element] = NULL;
   ...
   +      $pager_limits[$element] = NULL;
   

   These all need to be set up correctly for BC to be maintained but it does not look as thought they are.

3. +++ b/core/lib/Drupal/Core/Pager/Pager.php
   @@ -0,0 +1,238 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function init($total, $limit) {
   +    // The total number of pages to be managed by the pager is given by the
   +    // total number of items ($total) divided by the number of items in each
   +    // page ($limit). We round up to the next integer to accommodate any
   +    // remainder.
   +    $this
   +      ->setTotalItems($total)
   +      ->setTotalPages((int) ceil($this->getTotalItems() / $limit))
   +      ->setCurrentPage(max(0, min($this->getCurrentPage(), ((int) $this->getTotalPages() - 1))))
   +      ->setLimit($limit);
   +    return $this;
   +  }
   

   Why is this not part of construction? What's the point of an uninitialised pager?

4. +++ b/core/lib/Drupal/Core/Pager/Pager.php
   @@ -0,0 +1,238 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setCurrentPage($page) {
   +    $this->currentPage = (int) $page;
   +    return $this;
   +  }
   

   This should validate that it is a possible page.

5. +++ b/core/lib/Drupal/Core/Pager/Pager.php
   @@ -0,0 +1,238 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setTotalPages($total_pages) {
   +    $this->totalPages = (int) $total_pages;
   +    return $this;
   +  }
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setTotalItems($total_items) {
   +    $this->totalItems = (int) $total_items;
   +    return $this;
   +  }
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setLimit($limit) {
   +    $this->limit = (int) $limit;
   +    return $this;
   +  }
   

   I don't think this should be changeable after construction.

Looks good @mondrake. I think that that all points of @alexpott are addressed. I do have some other points:

1. +++ b/core/includes/pager.inc
   @@ -123,18 +120,15 @@ function pager_find_page($element = 0) {
   -  $pager_total_items[$element] = $total;
   -  $pager_total[$element] = ceil($pager_total_items[$element] / $limit);
   -  $pager_page_array[$element] = max(0, min($page, ((int) $pager_total[$element]) - 1));
   -  $pager_limits[$element] = $limit;
   

   Here are the pager global variables set and we are removing that. The problem for me is that AFAIK in the Pager service they are not set anywhere.

2. +++ b/core/lib/Drupal/Core/Pager/Pager.php
   @@ -0,0 +1,233 @@
   +      ->setTotalPages((int) ceil($this->getTotalItems() / $limit))
   ...
   +      ->setLimit($limit);
   

   Can we move the line ->setLimit($limit); 2 lines up. So that we can replace $limit with $this->getLimit() in the other line.

3. +++ b/core/lib/Drupal/Core/Pager/PagerFactory.php
   @@ -0,0 +1,158 @@
   +      $this->pagers[$element] = $this->create($element, $total, $limit);
   

   Why are we using the protected method create? Can we not just call new Pager() directly. Can we then remove the method create?

4. +++ b/core/lib/Drupal/Core/Pager/PagerFactory.php
   @@ -0,0 +1,158 @@
   +    // max(array_keys($this->pagers)) directly.
   

   Nitpick: should be "// max(array_keys($this->pagers)) + 1 directly."

5. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
   @@ -77,7 +77,7 @@ public function getFormId() {
   -    global $pager_page_array, $pager_total, $pager_total_items;
   +    global $pager_page_array, $pager_total, $pager_total_items, $pager_limits;
   
   @@ -173,6 +173,7 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
   +    $pager_limits[0] = $page_increment;
   

   I am missing why this is in this patch. If you found a bug in OverviewTerms, then create a new issue for it. It will then also need testing for the bug.

@mondrake: Thank for your new patch and your responce in comment #133/#134. For points 2-5 thank for fixing/explaining.

1. For point 1: I do not agree with you or I am missing something completely.
   If I take the line $this->limit = &$pager_limits[$element]; then AFAIK the value $this->limit is set to &$pager_limits[$element]; and that is it. The value of &$pager_limits[$element]; does not change in this line. For me the point from comment #132.1 still stands.
2. In this patch we creating the Pager service and I think we do not need the changes in core/modules/views/views.theme.inc for that.

Now do I get it. In this part are the variables linked to each other:

+++ b/core/lib/Drupal/Core/Pager/Pager.php
@@ -0,0 +1,233 @@
+    $this->currentPage = &$pager_page_array[$element];
+    $this->totalPages = &$pager_total[$element];
+    $this->totalItems = &$pager_total_items[$element];
+    $this->limit = &$pager_limits[$element];

And in the next part are the values set:

+++ b/core/lib/Drupal/Core/Pager/Pager.php
@@ -0,0 +1,233 @@
+    $this->setTotalItems($total)->setLimit($limit);
+    if ($this->getLimit() !== 0) {
+      $this->setTotalPages((int) ceil($this->getTotalItems() / $this->getLimit()));
+    }
+    $this->setCurrentPage(max(0, min($this->getCurrentPage(), ((int) $this->getTotalPages() - 1))));

Sorry for being a bit slow.

I am happy with the patch now and all of @alexpott points are addressed. So back to RTBC.

+++ b/core/includes/pager.inc
@@ -143,13 +137,16 @@ function pager_default_initialize($total, $limit, $element = 0) {
+ *
+ * @deprecated as of Drupal 8.3.x, will be removed before Drupal 9.0.0.
+ *   Use
+ *   \Drupal::service('pager.factory')->getCurrentRequestQueryParameters(['page'])
+ *   instead.
  */
 function pager_get_query_parameters() {
-  $query = &drupal_static(__FUNCTION__);
-  if (!isset($query)) {
-    $query = UrlHelper::filterQueryParameters(\Drupal::request()->query->all(), array('page'));
-  }
-  return $query;
+  $pager_factory = \Drupal::service('pager.factory');
+  $pager_parameter = $pager_factory->getPagerParameterName();
+  return $pager_factory->getCurrentRequestQueryParameters([$pager_parameter]);

+++ b/core/lib/Drupal/Core/Pager/PagerFactory.php
@@ -0,0 +1,159 @@
+  public function getCurrentRequestQueryParameter($parameter) {
...
+  public function getCurrentRequestQueryParameters(array $filter = []) {

Are these really necessary as public API it feel totally wrong to be exposing this here. The only external use is in page_get_query_paramaters() and afaics we should have a method that does this on the factory and not these other methods. Also you are removing the static cache - why? Furthermore if we moved all of this internal to the factory it wouldn't be necessary to expose the getPagerParameterName() either. Hence the entire implementation would become internal to the factory and the implementation details would not be being exposed by the factory interface.

All points of comment #139 from @alexpott are addressed. And it all looks good to me. So back to RTBC.

Back to RTBC.

bump - has been ready for 3 months?

+++ b/core/includes/pager.inc
@@ -23,15 +21,14 @@
+  return \Drupal::service('pager.factory')->findCurrentPage($element);

The service name doesn't seem right - it's not a factory if you are working directly with it? Looks like it more like a pager manager?

Also - should $element default to 0 in some or all of the methods? Seems you are relying on the the functional wrappers for that. The name "element" also seems a little odd and confusing in terms of form or html elements. why node something clearer like $pager_id?

The testbot failures are not related to this patch. So, back to RTBC.