Convert select query extenders to backend-overrideable services

Looks like duplicate of #3086647: Allow Select query builder to use class resolver so we can use dependency injection

Patch still applies and needs CR update

The patch looks good, almost RTBC.
I have created the text for the CR and I have updated the IS.

All the code changes look good to me.
For all the code changes is testing added.
There are 2 deprecations with deprecation message testing.
For me it is RTBC.

@mondrake: I am happy to review.

The testbot is now happy with PostgreSQL and SQLite.
The change looks good to me.
Back to RTBC.

@mondrake: Any idea why the MR was failing for PostgreSQL and SQLite?

I don't think the deprecations here warrant being Drupal 11. The API is not a really common one to use. Also we need a patch for Drupal 10 as the MR doesn't apply there - maybe the D10 patch could have the deprecations already removed.

@mondrake I think given we have a separate D10 patch that patch should not have the deprecations at all - there's no need.

1. +++ b/core/lib/Drupal/Core/Database/Query/PagerSelectExtenderFactory.php
   @@ -0,0 +1,45 @@
   +  /**
   +   * The pager manager service.
   +   *
   +   * @var \Drupal\Core\Pager\PagerManagerInterface
   +   */
   +  protected $pagerManager;
   +
   +  /**
   +   * Constructs a PagerSelectExtenderFactory object.
   +   *
   +   * @param \Drupal\Core\Pager\PagerManagerInterface $pager_manager
   +   *   The pager manager service.
   +   */
   +  public function __construct(PagerManagerInterface $pager_manager) {
   +    $this->pagerManager = $pager_manager;
   +  }
   

   Can use property promotion in D10.

2. +++ b/core/lib/Drupal/Core/Database/Query/TableSortExtender.php
   @@ -11,10 +12,29 @@
   +   * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack
   +   *   The request stack.
   ...
   -  public function __construct(SelectInterface $query, Connection $connection) {
   +  public function __construct(SelectInterface $query, Connection $connection, RequestStack $request_stack = NULL) {
   
   +++ b/core/lib/Drupal/Core/Database/Query/TableSortExtenderFactory.php
   @@ -0,0 +1,45 @@
   +  /**
   +   * The request stack.
   +   *
   +   * @var \Symfony\Component\HttpFoundation\RequestStack
   +   */
   +  protected $requestStack;
   +
   +  /**
   +   * Constructs a TableSortExtenderFactory object.
   +   *
   +   * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack
   +   *   The request stack.
   +   */
   +  public function __construct(RequestStack $request_stack) {
   +    $this->requestStack = $request_stack;
   +  }
   
   +++ b/core/modules/search/src/SearchQuery.php
   @@ -186,6 +202,32 @@ class SearchQuery extends SelectExtender {
   +  /**
   +   * Constructs a TableSortExtender object.
   +   *
   +   * @param \Drupal\Core\Database\Query\SelectInterface $query
   +   *   Select query object.
   +   * @param \Drupal\Core\Database\Connection $connection
   +   *   Database connection object.
   +   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
   +   *   The config factory.
   +   * @param \Drupal\search\SearchTextProcessorInterface $search_text_processor
   +   *   The search text processor service.
   +   */
   +  public function __construct(SelectInterface $query, Connection $connection, ConfigFactoryInterface $config_factory = NULL, SearchTextProcessorInterface $search_text_processor = NULL) {
   
   +++ b/core/modules/search/src/SearchQueryFactory.php
   @@ -0,0 +1,56 @@
   +  /**
   +   * Constructs a SearchQueryFactory object.
   +   *
   +   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
   +   *   The config factory.
   +   * @param \Drupal\search\SearchTextProcessorInterface $search_text_processor
   +   *   The search text processor service.
   +   */
   +  public function __construct(ConfigFactoryInterface $config_factory, SearchTextProcessorInterface $search_text_processor) {
   +    $this->configFactory = $config_factory;
   +    $this->searchTextProcessor = $search_text_processor;
   +  }
   
   +++ b/core/modules/search/src/ViewsSearchQueryFactory.php
   @@ -0,0 +1,56 @@
   +  /**
   +   * Constructs a ViewsSearchQueryFactory object.
   +   *
   +   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
   +   *   The config factory.
   +   * @param \Drupal\search\SearchTextProcessorInterface $search_text_processor
   +   *   The search text processor service.
   +   */
   +  public function __construct(ConfigFactoryInterface $config_factory, SearchTextProcessorInterface $search_text_processor) {
   +    $this->configFactory = $config_factory;
   +    $this->searchTextProcessor = $search_text_processor;
   +  }
   

   Same here - all this can use use protected property promotion. See https://stitcher.io/blog/constructor-promotion-in-php-8

The patch for D10 really cleans up a lot! :-)
Back to RTBC.

@mondrake: Great work.

Committed d0f7178 and pushed to 10.0.x. Thanks!
Committed 96cfccf and pushed to 9.4.x. Thanks!

Thank you!!! Published CR https://www.drupal.org/node/3218001

Reverting to fix two things:

1. +++ b/core/lib/Drupal/Core/Database/Query/PagerSelectExtenderFactory.php
   @@ -0,0 +1,39 @@
   +  public function get(SelectInterface $query, Connection $connection): PagerSelectExtender {
   
   +++ b/core/lib/Drupal/Core/Database/Query/TableSortExtenderFactory.php
   @@ -0,0 +1,39 @@
   +  public function get(SelectInterface $query, Connection $connection): TableSortExtender {
   
   +++ b/core/modules/search/src/SearchQueryFactory.php
   @@ -0,0 +1,43 @@
   +  public function get(SelectInterface $query, Connection $connection): SearchQuery {
   
   +++ b/core/modules/search/src/ViewsSearchQueryFactory.php
   @@ -0,0 +1,43 @@
   +  public function get(SelectInterface $query, Connection $connection): ViewsSearchQuery {
   

Our current coding standards specify type-hinting interfaces (not classes except under special circumstances). #3260507: Improve the discoverability of select extenders is trying to abstract a service for these, but it can't provide an interface for them with any return typehint because the specific return typehints cannot be compatible with a different return typehint on the parent. (PHP 7.4 fixes this, but Drupal 9 still has to not have fatals on PHP 7.3.)

For now, let's remove the return typehints, at least from Drupal 9.4. They can typehint the interface if we do #3260507: Improve the discoverability of select extenders. (Drupal 10 could still potentially have the specific class typehints, but I haven't worked through yet whether 9.4 would need to raise a deprecation warning somewhere if children of the individual getters returned other than their specific type or not. Please discuss!)

2. +++ b/core/modules/search/src/SearchQueryFactory.php
   @@ -0,0 +1,43 @@
   +  public function __construct(
   +    protected ConfigFactoryInterface $configFactory,
   +    protected SearchTextProcessorInterface $searchTextProcessor
   +  ) {
   +  }
   

   Here (and elsewhere) in the D10 patch, the method formatting violates our coding standards. The method signature should be on a single line. (Edit: And the curlies too, for an empty method.) TBH I was surprised we don't already have a phpcs rule enabled for that. :)

Thanks!

I'm also not totally sure we should be introducing protected property promotion yet (cool as it is); it adds a bit of delay to the contribution process and makes backports a bit harder. This type of thing didn't come up during 8.9/9.0 development because API additions were frozen at the time. We don't have a documented best practice one way or the other, though, so I brought this up with the other committers for additional feedback.

Hold off on addressing #45 for now as well -- folks are discussing whether the followup is desirable. Would love input from the folks who contributed to this issue on it as well.

The added interfaces are a bit more formal, only every new interface is going to be used only once. As somebody who will overrides query extenders, I am not going to use those new interfaces. I will extend the PagerSelectExtenderFactory class and extend the PagerSelectExtender class. For me the new interfaces are not necessary. I am curous what @xjm thinks of the added interfaces.

@mondrake well we don't have to do constructor promotion in here - we can always do it the old way and then convert in that issue. If something is 10.0.x only imo it should use constructor promotion today. I still think it is okay to use it in 10.x now and have a different patch for 9.x - the linked issue is about applying a rector rule to all of core. That's a completely different scope. But in order to proceed here let's just go the extra code and keep 9.x and 10.x the same.

The bigger issue here is how to proceed given the architecture question that emerged after the previous commit.

Ugh really sorry this didn't get back in for 9.4; I had hoped this would only be a couple weeks to sort out how to address the followup's needs without committing us permanently to a new API where BC would be an issue. In a way I guess it worked as intended because we didn't come up with a compromise in time for 9.4.0-beta1, but unfortunate anyway.

I think #49 and #52 raise good points. (The framework managers raised similar points when we discussed this issue after the revert.) The question is, is there a different way we could address the discoverability concerns raised in the related issue without the interface pollution, as it were?

Also a related task that is still outstanding is to formally amend our coding standards to not require interfaces when it doesn't make sense to have interfaces, and replace it with basic guidance about when to use an interface or not. We sort of agreed to do that when we discussed this issue earlier, but haven't yet implemented it (and it's difficult to do so when the TWG is defunct, but since this is about architecture I think in this case framework manager agreement on a new standard would be sufficient).

The solution looks good to me.

The last couple of nitpicks.
After this it is RTBC for me.

All code changes look good to me.
All my points have been addressed.
The IS and the CR are in order.
For me it is RTBC.

I'm triaging RTBC issues. I workd on this a few days ago as well.

I corrected a typo in the release snippet I saw when reading the Issue Summary.

I read the comments the other day and refreshed my memory just now. I did not find any unanswered queries there. The only thing is the followup to create at some point.

I am leaving this at RTBC.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Updated the branch/version on the CR. I did not review the CR.

The followup is still needed. It's a followup, not a blocker, so we can still go ahead here. We've also made coding standards progress on allowing the protected property promotion and the multi-line constructor format.

We do still need that additional framework manager feedback.

Meanwhile, there are various quite small coding standards issues with the MR, plus any number of inline comments we don't actually need. Plus, the scope of the D11 removal of the BC layer is sufficiently beyond a simple deprecation that we should go ahead and file the D11 followup now. (An existing issue was closed as outdated -- either it should be reopened and rescoped to be a part of the D11 beta requirements, or it should be replaced by an issue that is.)

Thanks for keeping this moving forward!