Add optional parameters to StatementInterface::fetchObject() to be in line with the PDO implementation of the method fetchObject()

The patch looks good.
The change are inline with how the method fetchObject() is used.
The 2 parameters are also part of PDOStatement::fetchObject(). See: https://www.php.net/manual/en/pdostatement.fetchobject.php.
There are no other implementations of the method fetchObject().
For me it is RTBC.

Good find @johnwebdev!

So:The fact that the parameters are optional allows us to avoid a BC break. However, I'd be interested to know why these parameter were not part of the inteface before, and are not part of several child implementations of fetchObject() nor of the other fetch methods. Maybe time to do a little git blame.

Anyone up to look into that and decide why this is worth doing for these few cases in the API, and why the one case of it was added in the first place?

Also, where's the test coverage?

Finally, if we do go ahead with this issue, it should get a change record as well.

Thanks for your consideration!

Updated the IS and created a CR.

Also, where's the test coverage?

Test coverage was added in #2956556: class isn't set in FETCH_OBJECT when class_name isn't set. The only test coverage we are missing is with the second parameter.

I think we should add those parameters as well in StatementWrapper like the following.

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

  public function fetchObject(string $class_name = NULL, $constructor_args = []) {
    if ($class_name) {
      return $this->clientStatement->fetchObject($class_name, $constructor_args);
    }
    return $this->clientStatement->fetchObject();
  }

Worked after applying patch in #3 and removing 'string' from the first line in #11

public function fetchObject($class_name = NULL, $constructor_args = []) {

@DuneBL I don't understand why we'd want to do the conditional you mentioned in #11 Edit, ignore me, that conditional is already there... rolling a patch

Here's a patch from #11 and #13 thank you both it's working.

The interface now matches \Drupal\Core\Database\StatementPrefetch::fetchObject

#8/#9 cover where the tests are at. FakeRecord::class doesn't take constructor args, is it ok to add a constructor that class, or do we need to create a new one for the test?

FakeRecord::class doesn't take constructor args, is it ok to add a constructor that class, or do we need to create a new one for the test?

I have no problems with adding a class variable to the class FakeRecord. Something like:

  /**
   * A class variable.
   *
   * @var integer
   */
  public $variable;

  /**
   * Constructs a FakeRecord object.
   *
   * @param integer $variable
   *   A class variable.
   */
  public function __construct($variable = 0) {
    $this->variable = $variable;
  }

The test Drupal\KernelTests\Core\Database\FetchTest::testQueryFetchObjectClass() can then add a constructor argument and also add an assertion to make sure variable: $result->variable is equal to the given constructor argument.

I've tested patch #15 on Drupal 9.1.0 and fixed some problems created for ultimate_cron (https://www.drupal.org/project/ultimate_cron/issues/3184623#comment-1392...)

thanks guys! patch #15 works in my site after the drupal core update from 9.0.9 to 9.1.0 :) i hope future updates works better than this :/

Hello,

I am working on a patch for this.

I'm mentoring yonailo on this patch. I added the Novice tag and Europe2020 tags.

Here there are 2 patches :

• 3128548-20-StatementInterface.patch : the patch from #15 plus the additional test inside testQueryFetchObjectClass()
• 3128548-20-StatementInterface-test-only.patch : a test-only patch, which should fail without the previous one.

Test looks good, @yonailo! Thank you.

I removed the Needs tests tag because @yonailo added a test, but I'm going to change this back to Needs work for some minor code standard issues with the patch in #21:

1. +++ b/core/modules/system/tests/src/Functional/Database/FakeRecord.php
   @@ -9,4 +9,22 @@
   +   * @var integer
   ...
   +   * @param integer $fakeArg
   

   I don't thinks matters too much, but we prefer the short name for variables.

2. +++ b/core/tests/Drupal/KernelTests/Core/Database/FetchTest.php
   @@ -88,10 +88,11 @@ public function testQueryFetchClass() {
   +      $this->assertTrue($result->fakeArg == 1, "The record has received an argument through its constructor.");
   

   We should use single quotes rather than double quotes because we don't need string interpolation.

+++ b/core/tests/Drupal/KernelTests/Core/Database/FetchTest.php
@@ -88,10 +88,11 @@ public function testQueryFetchClass() {
+      $this->assertTrue($result->fakeArg == 1, "The record has received an argument through its constructor.");

I think using assertSame(1, $result->fakeArg) would be preferable here.

Hello,

Thank you very much for taking your time for reviewing.

I have applied your suggestions and re-uploaded the files. I hope this time is all good :)

The added test looks good.
As all the other changes in the patch.
There is a CR added for the API addition.
The patch does what it should do according to the IS.
For me it is RTBC.

Re-uploading the patch to be committed. So that the testbot does not fail on the patch without the fix.

Thank you :)
Patch #28 is working with Drupal 9.1.x too

So:The fact that the parameters are optional allows us to avoid a BC break.

I think we need to grep contrib and see if anyone is implementing StatementInterface directly, and open issues if so, because it'll still break that case - just not for calling code. Change seems fine to me under the 1-1 rule in 9.2.x.

As requested by @catch I did search for the use of the method Statement::fetchObject() with the use of parameters in core and I found 2 modules who are using it.
The first is the Oracle database driver that is overriding the default implementation of the method in the same way as we are doing here, only with different parameter names. The project information indicates that nobody is using the Drupal 8 version of this module.
The second is the ultimate_cron module which is using it in the way we are implementing it. See: https://git.drupalcode.org/project/ultimate_cron/-/blob/8.x-2.x/src/Plug.... The Drupal 8 version of that module is used by 33.000 sites, according the projects info page.
I have contacted maintainers from both projects and I am waiting for their replies.

Fixing this issue will fix a bug on the ultimate_cron module. See: #3184623: ArgumentCountError: Too few arguments to function LogEntry::__construct().

+++ b/core/lib/Drupal/Core/Database/StatementInterface.php
@@ -127,8 +127,17 @@ public function fetchField($index = 0);
    * The object will be of the class specified by StatementInterface::setFetchMode()
    * or stdClass if not specified.
+   *
+   * @param string|null $class_name
+   *   Name of the created class.
+   * @param array $constructor_args
+   *   Elements of this array are passed to the constructor.
+   *
+   * @return mixed
+   *   The object of specified class or \stdClass if not specified. Returns
+   *   FALSE or NULL if there is no next row.
    */
-  public function fetchObject();
+  public function fetchObject($class_name = NULL, $constructor_args = []);
 

I think we should be adding these parameters commented out, with a note that implementing them will be required in Drupal 10 (and a follow-up to fix that) - this is what Symfony has done for adding optional parameters to interface methods. Oracle might not have a Drupal 9 release yet, but it shows that the current change could break contrib drivers.

Presumably this would still fix things for ultimate_cron since the implementations will accept the extra arguments. Also discussed with @xjm who confirmed that #5 didn't mean adding them to the interface now, but finding out why they weren't before.

Patch for comment on #33. I tested that after removal of the parameters on StatementInterface::fetchObject(), ultimate_cron module issue is resolved.

Missing patch. Couldn't provide an interdiff because ran into interdiff: Whitespace damage detected in input error.

The testbot failed with:

FILE: ...core/modules/system/tests/src/Functional/Database/FakeRecord.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 29 | ERROR | [x] Expected 1 blank line after function; 0 found
 30 | ERROR | [x] The closing brace for the class must have an empty
    |       |     line before it

Resolving issues from #36.

The points of @catch have been addressed.
Back to RTBC.

@sokru: Thanks.

1. +++ b/core/lib/Drupal/Core/Database/StatementEmpty.php
   @@ -68,7 +68,7 @@ public function fetchField($index = 0) {
   -  public function fetchObject() {
   +  public function fetchObject($class_name = NULL, $constructor_args = []) {
   

   Are we sure about [] as the default value... see PHP 8.0 code...
   public function fetchObject(?string $class = "stdClass", ?array $ctorArgs = null) {}
   https://github.com/php/php-src/blob/1954e5975846b3952ce1d2d6506e6d7134c8...

2. +++ b/core/lib/Drupal/Core/Database/StatementInterface.php
   @@ -127,6 +127,18 @@ public function fetchField($index = 0);
   +   *
   +   * @todo These parameters are not implemented. They should be added in Drupal 10.
   +   * https://www.drupal.org/node/3194677.
   +   *
   +   * // @param string|null $class_name
   +   *   Name of the created class.
   +   * // @param array $constructor_args
   +   *   Elements of this array are passed to the constructor.
   

   We can do a bit better here. Something like:

      * phpcs:disable Drupal.Commenting
      * @todo Remove PHPCS overrides https://www.drupal.org/node/3194677.
      *
      * @param string|null $class_name
      *   Name of the created class.
      * @param array $constructor_args
      *   Elements of this array are passed to the constructor.
      * phpcs:enable
   

   Will make it skip the phpcs errors whilst still providing better IDE documentation.

3. +++ b/core/lib/Drupal/Core/Database/StatementWrapper.php
   @@ -190,9 +190,9 @@ public function fetchAssoc() {
   -  public function fetchObject(string $class_name = NULL) {
   +  public function fetchObject($class_name = NULL, $constructor_args = []) {
   

   Seems a shame to be removing the scalar type hint. Is that necessary?

MAde suggested changes in #39.
Also changed argument name from $constructor_args to $constructor_arguments to keep it uniform as used in

+++ b/core/lib/Drupal/Core/Database/StatementEmpty.php
  /**
   * {@inheritdoc}
   */
  public function fetchAll($mode = NULL, $column_index = NULL, $constructor_arguments = NULL) {
    return [];
  }

Forgot to update the argument name in StatementInterface.php

1. +++ b/core/lib/Drupal/Core/Database/StatementEmpty.php
   @@ -68,7 +68,7 @@ public function fetchField($index = 0) {
   +  public function fetchObject(?string $class_name = NULL, ?array $constructor_arguments = NULL) {
   
   +++ b/core/lib/Drupal/Core/Database/StatementWrapper.php
   @@ -190,9 +190,9 @@ public function fetchAssoc() {
   +  public function fetchObject(?string $class_name = NULL, ?array $constructor_arguments = NULL) {
   

   Can we remove ?string and ?array.

2. +++ b/core/lib/Drupal/Core/Database/StatementInterface.php
   @@ -127,8 +127,21 @@ public function fetchField($index = 0);
   +   * @param array $constructor_arguments
   

   Can we change this to: * @param array|null $constructor_arguments. The default value is NULL.

3. For #39.3: According to @catch it is. For D9 at least, for BC reasons. In D10 we can add the PHP8 version in.

Addressed #45.

+++ b/core/lib/Drupal/Core/Database/StatementInterface.php
@@ -127,8 +127,21 @@ public function fetchField($index = 0);
-  public function fetchObject();
+  public function fetchObject($class_name = NULL, $constructor_arguments = NULL);

How come we're now adding the param to the interface? I thought the idea was to add them properly in D10? This is a breaking change. If we don't add them here we can add the correct typehints to the implementation and not remove the string typehint.

Also in PHP 7 the constructor arguments is typehinted with array as far as I can see.

I think we should be adding string $class_name = NULL, array $constructor_arguments = NULL to all of the concrete implementations and not adding arguments to the interface until D10.

Doing what Symfony does would make the interface look like:

  /**
   * Fetches the next row and returns it as an object.
   *
   * The object will be of the class specified by StatementInterface::setFetchMode()
   * or stdClass if not specified.
   *
   * phpcs:disable Drupal.Commenting
   * @todo Remove PHPCS overrides https://www.drupal.org/node/3194677.
   *
   * @param string|null $class_name
   *   Name of the created class.
   * @param array|null $constructor_arguments
   *   Elements of this array are passed to the constructor.
   * phpcs:enable
   *
   * @return mixed
   *   The object of specified class or \stdClass if not specified. Returns
   *   FALSE or NULL if there is no next row.
   */
  public function fetchObject(/** string $class_name = NULL, array $constructor_arguments = NULL */);

which is what I think we want to do. Then anything in contrib can copy the concrete implementations in core and implement the new interface now and not have to change anything in D10.

Addressing #47.

Fixed the remaining failing test cases.

Fixed in the way that @alexpott wants.
Back to RTBC.

1. +++ b/core/lib/Drupal/Core/Database/StatementEmpty.php
   @@ -68,7 +68,7 @@ public function fetchField($index = 0) {
   -  public function fetchObject() {
   +  public function fetchObject($class_name = NULL, $constructor_arguments = NULL) {
   

   Lets add the string and array typehints.

2. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
   @@ -418,7 +418,7 @@ public function fetchField($index = 0) {
   -  public function fetchObject($class_name = NULL, $constructor_args = []) {
   +  public function fetchObject($class_name = NULL, $constructor_arguments = NULL) {
   

   I think we can add the typehints.

3. +++ b/core/lib/Drupal/Core/Database/StatementWrapper.php
   @@ -190,9 +190,9 @@ public function fetchAssoc() {
   -  public function fetchObject(string $class_name = NULL) {
   +  public function fetchObject($class_name = NULL, $constructor_arguments = NULL) {
   

   There is no need for the string typehint to be removed here. In fact I would argue we can add the array typehint here.

Added the type hints.

All points of @alexpott have been addressed.
Back to RTBC.

+++ b/core/lib/Drupal/Core/Database/StatementEmpty.php
@@ -68,7 +68,7 @@ public function fetchField($index = 0) {
+  public function fetchObject(?string $class_name = NULL, ?array $constructor_arguments = NULL) {

I do not think we should also add the nullability operator here. In other words, you will not explicitly pass a NULL here, only fallback to NULL if you won't pass one, so

  public function fetchObject(string $class_name = NULL, array $constructor_arguments = NULL) {

would be fine IMHO.

BTW - In some way this goes in the opposite direction of trying to decouple the db API from PDO, which we are trying in other issues. Not a big deal really, but wanted to mention this.

+++ b/core/lib/Drupal/Core/Database/StatementEmpty.php
@@ -68,7 +68,7 @@ public function fetchField($index = 0) {
+  public function fetchObject(?string $class_name = NULL, ?array $constructor_arguments = NULL) {

+++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
@@ -418,7 +418,7 @@ public function fetchField($index = 0) {
+  public function fetchObject(?string $class_name = NULL, ?array $constructor_arguments = NULL) {

+++ b/core/lib/Drupal/Core/Database/StatementWrapper.php
@@ -190,9 +190,9 @@ public function fetchAssoc() {
+  public function fetchObject(?string $class_name = NULL, ?array $constructor_arguments = NULL) {

?string $var = NULL - either the ? or the = NULL is redundant. I think I would do string $var = NULL as the question mark syntax is relatively recent. And is not used in the commented out text on the interface.

And the question marks have been removed.
Back to RTBC.

Committed cc41d94 and pushed to 9.2.x. Thanks!

I tested the latest patch with Ultimate Cron and it fixes the module. Going to ping release managers about backporting.

+++ b/core/lib/Drupal/Core/Database/StatementEmpty.php
@@ -68,7 +68,7 @@ public function fetchField($index = 0) {
+  public function fetchObject(string $class_name = NULL, array $constructor_arguments = NULL) {

+++ b/core/lib/Drupal/Core/Database/StatementInterface.php
@@ -127,8 +127,21 @@ public function fetchField($index = 0);
+  public function fetchObject(/* string $class_name = NULL, array $constructor_arguments = NULL */);

+++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
@@ -418,7 +418,7 @@ public function fetchField($index = 0) {
+  public function fetchObject(string $class_name = NULL, array $constructor_arguments = NULL) {

+++ b/core/lib/Drupal/Core/Database/StatementWrapper.php
@@ -190,9 +190,9 @@ public function fetchAssoc() {
+  public function fetchObject(string $class_name = NULL, array $constructor_arguments = NULL) {

I considered asking for $class_name to be defaulted to "stdClass" as per https://github.com/php/php-src/blob/1954e5975846b3952ce1d2d6506e6d7134c8...

But reading the code - https://github.com/php/php-src/blob/1954e5975846b3952ce1d2d6506e6d7134c8... - this param is optional and it does the same thing.

Discussed with @catch - we agreed to backport this because it fixes a 9.1.x bug in a well used contrib module that can only be fixed in core.