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

Thanks for the report.

IMHO this is a core issue, specifically an oversight in the conversion of the pager to services in #2044435: Convert pager.inc to a service.

The current page number should be determined by calling ::findPage from the 'pager.parameters' service, not directly from the request object. Pagerer overrides the 'pager.parameters' service to return it from the custom querystring, but cannot do anything if the request is accessed directly.

Moving to core issue queue.

That's the idea, let's see if tests pass... I do not know how we could add specific testing for this though.

It's a good idea to put the medicament in the syringe, for an injection.

1. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
   @@ -93,6 +103,11 @@ public function __construct(ModuleHandlerInterface $module_handler, EntityTypeMa
   +      @trigger_error('Calling OverviewTerms::__construct() without passing the $pager_parameters argument is deprecated in drupal:9.1.0 and will be required in drupal:10.0.0. See https://www.drupal.org/node/TODO', E_USER_DEPRECATED);
   

   I don't think we need a CR here. So the link is not required.

2. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
   @@ -104,7 +119,8 @@ public static function create(ContainerInterface $container) {
   +      $container->get('pager.manager'),
   +      $container->get('pager.parameters')
   
   @@ -136,7 +152,7 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
   -    $page = $this->getRequest()->query->get('page') ?: 0;
   +    $page = $this->pagerParameters->findPage();
   

   This seems really awkward to have to inject both of these services. I think 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. I wonder if we should open a follow-up for that. Because then we could remove the service injection.

Filed follow-up #3173636: Implement a PagerManagerInterface::findPage() method, for D10 - we'd break BC for contrib if we did that earlier.

Fixed the @trigger_error.

Back to RTBC since the change is only to an untested deprecation error string.

+++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
@@ -93,6 +103,11 @@ public function __construct(ModuleHandlerInterface $module_handler, EntityTypeMa
+      @trigger_error('Calling OverviewTerms::__construct() without passing the $pager_parameters argument is deprecated in drupal:9.1.0 and will be required in drupal:10.0.0.', E_USER_DEPRECATED);
+      $pager_parameters = \Drupal::service('pager.parameters');

We would ordinarily require a deprecation test to go with a new deprecation

Can we get a simple test that checks the correct deprecation error is emitted in that scenario.

Not sure about adding a test only for the additional constructor arguments. At least #3096648: Add support for third party libraries in site specific and install profile specific libraries folders and #3101299: Install module from .zip URL fails did not introduce one. Can we clarify the policy before adding one?

Re #11 @catch and I have discussed this before and agreed that deprecation testing for only a constructor change like this is unnecessary. If there is more logic in the constructor than a simple is not null check then a test is required but that's not the case here.

Ok, I wasn't aware of that decision.

I'll update the 'how' section of the docs tomorrow

#3173636: Implement a PagerManagerInterface::findPage() method is making good progress, so I suggest to wait for that to go in, which will make this patch much simpler.

#3173636: Implement a PagerManagerInterface::findPage() method was committed, and now the fix is just a one-liner.

#3173636: Implement a PagerManagerInterface::findPage() method was committed, and now the fix is just a one-liner.

Confirm that patch #18 solves the problem with the right approach after #3173636: Implement a PagerManagerInterface::findPage() method was committed.
Moving to RTBC.

As a bug it needs -fail.patch steps in summary looks great for scenario

You can't test this unless you swap the service, which would require developing a test module for. Not sure it's worth it. Existing tests already work in the sense that the change is behaving (for core) exactly as if the page is extracted from the request.

Yes, I was trying to write tests for it but did not find a way. I just can confirm that it fixes the problem related on the issue summary.

Maybe screenshots are enough?

Agreed this is OK without an explicit test, it's a refactor to use the correct API that enables contrib to override properly.

Committed/pushed to 9.2.x, thanks!