Issue #1202484 by mondrake, raman.b, Michael Zetterberg fd. Lopez, Rajendar Reddy, daffie, catch, heddn, petrusek: Improve usage of 'pager.manager' service in PagerSelectExtender, allow code to know the pager element ID used

core/lib/Drupal/Core/Database/Connection.php

@@ -11,6 +11,7 @@ use Drupal\Core\Database\Query\Select;
 use Drupal\Core\Database\Query\Truncate;
 use Drupal\Core\Database\Query\Update;
 use Drupal\Core\Database\Query\Upsert;
+use Drupal\Core\Pager\PagerManagerInterface;
 
 /**
  * Base Database API class.
@@ -1968,4 +1969,17 @@ abstract class Connection {
     return ($first === 'Drupal' && strtolower($second) === $second) ? $second : 'core';
   }
 
+  /**
+   * Get the pager manager service, if available.
+   *
+   * @return \Drupal\Core\Pager\PagerManagerInterface
+   *   The pager manager service, if available.
+   *
+   * @throws \Drupal\Core\DependencyInjection\ContainerNotInitializedException
+   *   If the container has not been initialized yet.
+   */
+  public function getPagerManager(): PagerManagerInterface {
+    return \Drupal::service('pager.manager');
+  }
+
 }

core/lib/Drupal/Core/Database/Query/PagerSelectExtender.php

@@ -20,6 +20,11 @@ class PagerSelectExtender extends SelectExtender {
    * The highest element we've autogenerated so far.
    *
    * @var int
+   *
+   * @deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Use
+   *   \Drupal::service('pager.manager')->getMaxPagerElementId() instead.
+   *
+   * @see https://www.drupal.org/node/3194594
    */
   public static $maxElement = 0;
 
@@ -44,6 +49,14 @@ class PagerSelectExtender extends SelectExtender {
    */
   protected $customCountQuery = FALSE;
 
+  /**
+   * Constructs a PagerSelectExtender object.
+   *
+   * @param \Drupal\Core\Database\Query\SelectInterface $query
+   *   Select query object.
+   * @param \Drupal\Core\Database\Connection $connection
+   *   Database connection object.
+   */
   public function __construct(SelectInterface $query, Connection $connection) {
     parent::__construct($query, $connection);
 
@@ -73,9 +86,7 @@ class PagerSelectExtender extends SelectExtender {
     $this->ensureElement();
 
     $total_items = $this->getCountQuery()->execute()->fetchField();
-    /** @var \Drupal\Core\Pager\PagerManagerInterface $pager_manager */
-    $pager_manager = \Drupal::service('pager.manager');
-    $pager = $pager_manager->createPager($total_items, $this->limit, $this->element);
+    $pager = $this->connection->getPagerManager()->createPager($total_items, $this->limit, $this->element);
     $this->range($pager->getCurrentPage() * $this->limit, $this->limit);
 
     // Now that we've added our pager-based range instructions, run the query normally.
@@ -84,15 +95,13 @@ class PagerSelectExtender extends SelectExtender {
 
   /**
    * Ensure that there is an element associated with this query.
-   * If an element was not specified previously, then the value of the
-   * $maxElement counter is taken, after which the counter is incremented.
    *
    * 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++;
+      $this->element($this->connection->getPagerManager()->getMaxPagerElementId() + 1);
     }
   }
 
@@ -151,9 +160,6 @@ class PagerSelectExtender extends SelectExtender {
    * whatever reason you want to explicitly define an element for a given query,
    * you may do so here.
    *
-   * Setting the element here also increments the static $maxElement counter,
-   * which is used for determining the $element when there's none specified.
-   *
    * Note that no collision detection is done when setting an element ID
    * explicitly, so it is possible for two pagers to end up using the same ID
    * if both are set explicitly.
@@ -163,10 +169,23 @@ class PagerSelectExtender extends SelectExtender {
    */
   public function element($element) {
     $this->element = $element;
-    if ($element >= self::$maxElement) {
-      self::$maxElement = $element + 1;
-    }
+    $this->connection->getPagerManager()->reservePagerElementId($this->element);
     return $this;
   }
 
+  /**
+   * Gets the element ID for this pager query.
+   *
+   * The element is used to differentiate different pager queries on the same
+   * page so that they may be operated independently.
+   *
+   * @return int
+   *   Element ID that is used to differentiate between different pager
+   *   queries.
+   */
+  public function getElement(): int {
+    $this->ensureElement();
+    return $this->element;
+  }
+
 }

core/lib/Drupal/Core/Entity/Query/QueryBase.php

@@ -2,7 +2,6 @@
 
 namespace Drupal\Core\Entity\Query;
 
-use Drupal\Core\Database\Query\PagerSelectExtender;
 use Drupal\Core\Entity\EntityTypeInterface;
 use Drupal\Core\Utility\TableSort;
 
@@ -289,10 +288,7 @@ abstract class QueryBase implements QueryInterface {
     // Even when not using SQL, storing the element PagerSelectExtender is as
     // good as anywhere else.
     if (!isset($element)) {
-      $element = PagerSelectExtender::$maxElement++;
-    }
-    elseif ($element >= PagerSelectExtender::$maxElement) {
-      PagerSelectExtender::$maxElement = $element + 1;
+      $element = \Drupal::service('pager.manager')->getMaxPagerElementId() + 1;
     }
 
     $this->pager = [

core/lib/Drupal/Core/Pager/PagerManager.php

@@ -2,6 +2,7 @@
 
 namespace Drupal\Core\Pager;
 
+use Drupal\Core\Database\Query\PagerSelectExtender;
 use Drupal\Core\DependencyInjection\DependencySerializationTrait;
 
 /**
@@ -31,6 +32,13 @@ class PagerManager implements PagerManagerInterface {
    */
   protected $pagers;
 
+  /**
+   * The highest pager ID created so far.
+   *
+   * @var int
+   */
+  protected $maxPagerElementId = -1;
+
   /**
    * Construct a PagerManager object.
    *
@@ -90,13 +98,20 @@ class PagerManager implements PagerManagerInterface {
   }
 
   /**
-   * Gets the extent of the pager page element IDs.
-   *
-   * @return int
-   *   The maximum element ID available, -1 if there are no elements.
+   * {@inheritdoc}
    */
-  protected function getMaxPagerElementId() {
-    return empty($this->pagers) ? -1 : max(array_keys($this->pagers));
+  public function getMaxPagerElementId() {
+    return $this->maxPagerElementId;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function reservePagerElementId(int $element): void {
+    $this->maxPagerElementId = max($element, $this->maxPagerElementId);
+    // BC for PagerSelectExtender::$maxElement.
+    // @todo remove the line below in D10.
+    PagerSelectExtender::$maxElement = $this->getMaxPagerElementId();
   }
 
   /**
@@ -108,6 +123,7 @@ class PagerManager implements PagerManagerInterface {
    *   The pager index.
    */
   protected function setPager(Pager $pager, $element = 0) {
+    $this->maxPagerElementId = max($element, $this->maxPagerElementId);
     $this->pagers[$element] = $pager;
   }
 

core/lib/Drupal/Core/Pager/PagerManagerInterface.php

@@ -33,7 +33,7 @@ interface PagerManagerInterface {
    * before executing it. For example:
    * @code
    *   $query = $connection->select('some_table')
-   *     ->extend('Drupal\Core\Database\Query\PagerSelectExtender');
+   *     ->extend(PagerSelectExtender::class);
    * @endcode
    *
    * However, if you are using a different method for generating the items to be
@@ -163,4 +163,26 @@ interface PagerManagerInterface {
    */
   public function getUpdatedParameters(array $query, $element, $index);
 
+  /**
+   * Gets the extent of the pager page element IDs.
+   *
+   * @return int
+   *   The maximum element ID available, -1 if there are no elements.
+   */
+  public function getMaxPagerElementId();
+
+  /**
+   * Reserve a pager element ID.
+   *
+   * Calling code may need to reserve the ID of a pager before actually creating
+   * it. This methods allows to do so ensuring no collision occurs with
+   * ::getMaxPagerElementId().
+   *
+   * @param int $element
+   *   The ID of the pager to be reserved.
+   *
+   * @see \Drupal\Core\Database\Query\PagerSelectExtender::element()
+   */
+  public function reservePagerElementId(int $element): void;
+
 }

core/modules/system/tests/src/Functional/Database/SelectPagerDefaultTest.php

@@ -144,34 +144,46 @@ class SelectPagerDefaultTest extends DatabaseTestBase {
     \Drupal::getContainer()->get('request_stack')->push($request);
 
     $connection = Database::getConnection();
-    $name = $connection->select('test', 't')
+    $query = $connection->select('test', 't')
       ->extend(PagerSelectExtender::class)
       ->element(2)
       ->fields('t', ['name'])
       ->orderBy('age')
-      ->limit(1)
-      ->execute()
+      ->limit(1);
+    $this->assertSame(2, $query->getElement());
+    // BC for PagerSelectExtender::$maxElement.
+    // @todo remove the assertion below in D10.
+    $this->assertSame(2, PagerSelectExtender::$maxElement);
+    $name = $query->execute()
       ->fetchField();
     $this->assertEqual('Paul', $name, 'Pager query #1 with a specified element ID returned the correct results.');
 
-    // Setting an element smaller than the previous one
-    // should not overwrite the pager $maxElement with a smaller value.
-    $name = $connection->select('test', 't')
+    // Setting an element smaller than the previous one should not collide with
+    // the existing pager.
+    $query = $connection->select('test', 't')
       ->extend(PagerSelectExtender::class)
       ->element(1)
       ->fields('t', ['name'])
       ->orderBy('age')
-      ->limit(1)
-      ->execute()
+      ->limit(1);
+    $this->assertSame(1, $query->getElement());
+    // BC for PagerSelectExtender::$maxElement.
+    // @todo remove the assertion below in D10.
+    $this->assertSame(2, PagerSelectExtender::$maxElement);
+    $name = $query->execute()
       ->fetchField();
     $this->assertEqual('George', $name, 'Pager query #2 with a specified element ID returned the correct results.');
 
-    $name = $connection->select('test', 't')
+    $query = $connection->select('test', 't')
       ->extend(PagerSelectExtender::class)
       ->fields('t', ['name'])
       ->orderBy('age')
-      ->limit(1)
-      ->execute()
+      ->limit(1);
+    $this->assertSame(3, $query->getElement());
+    // BC for PagerSelectExtender::$maxElement.
+    // @todo remove the assertion below in D10.
+    $this->assertSame(3, PagerSelectExtender::$maxElement);
+    $name = $query->execute()
       ->fetchField();
     $this->assertEqual('John', $name, 'Pager query #3 with a generated element ID returned the correct results.');
 

core/tests/Drupal/KernelTests/Core/Database/SelectExtenderTest.php

@@ -1,20 +1,20 @@
 <?php
 
-namespace Drupal\Tests\Core\Database;
+namespace Drupal\KernelTests\Core\Database;
 
 use Composer\Autoload\ClassLoader;
 use Drupal\Core\Database\Query\SelectExtender;
+use Drupal\KernelTests\KernelTestBase;
 use Drupal\Tests\Core\Database\Stub\StubConnection;
 use Drupal\Tests\Core\Database\Stub\StubPDO;
-use Drupal\Tests\UnitTestCase;
 
 /**
- * Tests the Connection class.
+ * Tests the Select query extender classes.
  *
  * @coversDefaultClass \Drupal\Core\Database\Query\Select
  * @group Database
  */
-class SelectTest extends UnitTestCase {
+class SelectExtenderTest extends KernelTestBase {
 
   /**
    * Data provider for testExtend().
@@ -25,7 +25,7 @@ class SelectTest extends UnitTestCase {
    *   - The database driver namespace.
    *   - The namespaced class name for which to extend.
    */
-  public function providerExtend() {
+  public function providerExtend(): array {
     return [
       [
         'Drupal\Core\Database\Query\PagerSelectExtender',
@@ -115,7 +115,7 @@ class SelectTest extends UnitTestCase {
    * @covers \Drupal\Core\Database\Query\SelectExtender::extend
    * @dataProvider providerExtend
    */
-  public function testExtend($expected, $namespace, $extend) {
+  public function testExtend(string $expected, string $namespace, string $extend): void {
     $additional_class_loader = new ClassLoader();
     $additional_class_loader->addPsr4("Drupal\\corefake\\Driver\\Database\\corefake\\", __DIR__ . "/../../../../../tests/fixtures/database_drivers/module/corefake/src/Driver/Database/corefake");
     $additional_class_loader->addPsr4("Drupal\\corefake\\Driver\\Database\\corefakeWithAllCustomClasses\\", __DIR__ . "/../../../../../tests/fixtures/database_drivers/module/corefake/src/Driver/Database/corefakeWithAllCustomClasses");
@@ -130,7 +130,7 @@ class SelectTest extends UnitTestCase {
 
     // Get an instance of the class \Drupal\Core\Database\Query\SelectExtender.
     $select_extender = $connection->select('test')->extend(SelectExtender::class);
-    $this->assertEquals('Drupal\Core\Database\Query\SelectExtender', get_class($select_extender));
+    $this->assertEquals(SelectExtender::class, get_class($select_extender));
 
     // Tests the method \Drupal\Core\Database\Query\SelectExtender::extend().
     $select_extender_extended = $select_extender->extend($extend);

core/tests/Drupal/KernelTests/Core/Pager/PagerManagerTest.php

@@ -53,4 +53,36 @@ class PagerManagerTest extends KernelTestBase {
     $this->assertEquals(10, $pager_manager->findPage(1));
   }
 
+  /**
+   * @covers ::getMaxPagerElementId
+   *
+   * @dataProvider providerTestGetMaxPagerElementId
+   */
+  public function testGetMaxPagerElementId(array $elements, int $expected_max_element_id): void {
+    /* @var $pager_manager \Drupal\Core\Pager\PagerManagerInterface */
+    $pager_manager = $this->container->get('pager.manager');
+
+    foreach ($elements as $element) {
+      $pager_manager->createPager(30, 10, $element);
+    }
+
+    $this->assertEquals($expected_max_element_id, $pager_manager->getMaxPagerElementId());
+  }
+
+  /**
+   * Provides test cases for PagerManagerTest::testGetMaxPagerElementId().
+   *
+   * @return array
+   *   An array of test cases, each which the following values:
+   *   - Array of elements to pass to PagerManager::createPager().
+   *   - The expected value returned by PagerManager::getMaxPagerElementId().
+   */
+  public function providerTestGetMaxPagerElementId(): array {
+    return [
+      'no_pager' => [[], -1],
+      'single_pager' => [[0], 0],
+      'multiple_pagers' => [[30, 10, 20], 30],
+    ];
+  }
+
 }