Problem/Motivation

When calling fetchObject with a class name but without supplying constructor arguments, we get this warning:

Warning: PDOStatement::fetchObject() expects parameter 2 to be array, null given in Drupal\Core\Database\StatementWrapper->fetchObject()

This is because StatementWrapper defaults this to NULL, but PDOStatement expects an empty array if the argument is supplied.

Steps to reproduce

final class MyDatabaseObjectClass {
}

$queryResult = \Drupal::database()
  ->select('key_value', 'kv')
  ->fields('kv', ['name'])
  ->condition('collection', 'system.schema')
  ->condition('name', 'node')->execute();

$resultRowAsObject = $queryResult->fetchObject(MyDatabaseObjectClass::class);

Expected: An instance of MyDatabaseObjectClass
Actual: FALSE, plus a PHP Warning

Proposed resolution

Either the method signature of StatementWrapper::fetchObject should be changed to more closely align with PDOStatement::fetchObject, or a NULL guard should be added when passing $constructor_arguments to ensure an empty array is passed instead of NULL.

Release notes snippet

Any contrib database driver projects that implement StatementInterface::fetchObject will need to adjust their signature accordingly to prevent similar fatal errors.

CommentFileSizeAuthor
#10 3259740-10.patch4.91 KBacbramley
#7 3259740-7.patch1.93 KBacbramley
#7 3259740-7--test-only.patch1.21 KBacbramley

Comments

pstewart created an issue. See original summary.

pstewart’s picture

pstewart commented

For anyone else encountering this bug it can be worked around by passing an empty array explicitly, i.e. $resultRowAsObject = $queryResult->fetchObject(MyDatabaseObjectClass::class, [])

pstewart’s picture

pstewart commented

Forgot to mention the problem was encountered with Drupal 9.3.3 and PHP 7.4.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

acbramley’s picture

acbramley commented
Version: 9.5.x-dev » 11.x-dev
Issue tags: +Bug Smash Initiative

Triaged as part of BSI, bug still exists in 11.x

acbramley’s picture

acbramley commented
Status: Active » Needs review
StatusFileSize
new 3259740-7--test-only.patch1.21 KB
new 3259740-7.patch1.93 KB
acbramley’s picture

acbramley commented

I feel like the correct solution is array $constructor_arguments = [], but that may require a BC dance which it looks like these classes have already been through.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Database/StatementWrapperIterator.php
@@ -217,7 +217,7 @@ public function fetchAssoc() {
   public function fetchObject(string $class_name = NULL, array $constructor_arguments = NULL) {

I think it would be better to update fetchObject() to default to an empty array and trigger a deprecation warning if NULL is passed.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review
StatusFileSize
new 3259740-10.patch4.91 KB

I agree (see #8), let's see what others think.

The last submitted patch, 7: 3259740-7--test-only.patch, failed testing. View results

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Think #9 would be the safest. A trigger_error if null is passed in.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review

Explicitly passing NULL produces a similar error, therefore does not need a deprecation because it never worked in the first place.

TypeError: PDOStatement::fetchObject(): Argument #2 ($constructorArgs) must be of type array, null given in PDOStatement->fetchObject()
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Oh thanks for that @acbramley!

The last submitted patch, 7: 3259740-7--test-only.patch, failed testing. View results

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue tags: +Needs subsystem maintainer review

Going to ping daffie for a review

daffie’s picture

daffie commented
Status: Reviewed & tested by the community » Needs work

I think we need to add a deprecation warning when the second parameter is explicitly set to NULL and add testing for that.

daffie’s picture

daffie commented
Issue tags: -Needs subsystem maintainer review

I did the review, therefor I should have removed the tag.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review

I think we need to add a deprecation warning when the second parameter is explicitly set to NULL and add testing for that.

See #13 you can't explicitly pass NULL to the function, that fails with the same type error because the parameter is typed as an array.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Shouldn’t we fully align to the signature since we’re at it?

public PDOStatement::fetchObject(?string $class = "stdClass", array $constructorArgs = []): object|false

Maybe without the return typehint for now.

Not sure where we are with BC here - the interface is not typehinted yet, but concrete classes are and contrib extending from them would fail.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

After talking to @acbramley and @larowlan on Slack, do I get their point. The parent method from PHP PDO also does not work when the second parameter is set to NULL. Therefor that is not a BC break.

Not sure where we are with BC here - the interface is not typehinted yet, but concrete classes are and contrib extending from them would fail.

Yes this would be a BC break. We have 2 contrib database drivers (MySQLi and MS SQL Server). The first one is being integrated in Drupal core and the second one does not override the method. I do not know of any other contrib overrides for the method and http://grep.xnddx.ru is no longer working. I do not know of any other alternatives.
If there are any overrides it will be very few instances. For me it is an exceptable risk. I leave it up to committer to make up his/her own mind and make the final decession. Back to RTBC.

The last submitted patch, 7: 3259740-7--test-only.patch, failed testing. View results

  • larowlan committed 4dba7727 on 11.x 
    Issue #3259740 by acbramley, daffie: PDOStatement::fetchObject() expects...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Committed to 11.x

Created and published a change record

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.