Error when fetching all query results as class instances

Hi, in Drupal core changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies. Thanks.

@pjotr.savitski: Create a merge request if you have a fix and test ready in the branch. Then, depending on whether CI tests fail, you may need to rework your changes until all tests pass. Once that's done, change the status of this issue to "Needs review" and eventually someone in the community will take a look at it.

Left some comments on the MR thanks!

Feedback appears to be addressed.

Ran the test-only job and it confirms the error in the issue title:

There was 1 error:
1) Drupal\KernelTests\Core\Database\FetchTest::testQueryFetchAllAsClassInstances
TypeError: PDOStatement::fetchAll(): Argument #2 must be of type string, int given
/builds/issue/drupal-3552669/core/lib/Drupal/Core/Database/Statement/PdoTrait.php:191
/builds/issue/drupal-3552669/core/lib/Drupal/Core/Database/Statement/PdoResult.php:83
/builds/issue/drupal-3552669/core/lib/Drupal/Core/Database/Statement/StatementBase.php:339
/builds/issue/drupal-3552669/core/tests/Drupal/KernelTests/Core/Database/FetchTest.php:183

Going through a checklist of things for RTBC issues.

Thanks for the test!

Un-assigning per Assigning ownership of a Drupal core issue.

The proposed resolution in the issue summary has two options and says that the first is preferred. But the MR implements the second option. That should be explained in the issue summary to help reviewers or anyone researching this change.

The title here is an error statement but is should be a description of what is being fixed or improved. The title is used as the git commit message so it should be meaningful and concise. See List of issue fields. Since I can't think of a title right now I am tagging for that and setting to NW. Something about correctly handling the default column of '0'?

Merge request !14207 contains the first option by setting the column to NULL. The second option doesn't work with setFetchMode containing constructor args.

Re-based the MR, since it failed on the automated main re-target.

Tests pass.
Test only fails as expected: https://git.drupalcode.org/issue/drupal-3552669/-/jobs/8640046
IS and title look to have been updated per #19.

lgtm

I think we should move the fix to \Drupal\Core\Database\Statement\PdoResult::fetchAll as that's were this problem is coming from.

Something like:

  /**
   * {@inheritdoc}
   */
  public function fetchAll(FetchAs $mode, array $fetchOptions): array {
    $columnOrClass = $mode === FetchAs::ClassObject ? $fetchOptions['class'] ?? NULL : $fetchOptions['column'] ?? NULL;
    return $this->clientFetchAll($mode, $columnOrClass, $fetchOptions['constructor_args'] ?? NULL);
  }

Fixing it here would fix this for all calls to the public fetchAll().

Entity Hierarchy ran into this issue in https://git.drupalcode.org/project/entity_hierarchy/-/work_items/3590987, as it passed a class name to fetchAll($column_index), which broke in #3522561: Fully type StatementInterface methods' parameters. So we had to adapt to setFetchMode, which is how I ran into this new issue.

I adapted safety per @alexpott.
I wanted to improve a little typehinting but Coder is unhappy (despite PHPStan accepting it), so whatever.
Merged with todays main branch.

Looks good to me. Two tests added to prevent regressions.

Slight rework for @mondrake feedback.

Use the same assertEquals assertion strategy for both class properties.

Thanks

I've read the comments and updated credit (which does include me for earlier triage). I didn't find anything unanswered here.

Committed and pushed eeb5c183295 to main and 7c0abbbf045 to 11.x and 9282211f1a8 to 11.4.x. Thanks!