Fix the default value of $ctorArgs param in PDOStatement::fetchObject()

[kocsismate]

It looks like we don't have a real default value :(

[kocsismate]

sorry, I forgot to push my changes. Did I get it right that the behavior differs when the $ctorArgs param is provided and when it's not?

[kamil-tekiela]

My first thought: I think an empty array is an acceptable compromise. We can add a note in the manual about the unexpected behaviour with the default and temporary arguments but the signature can remain as an empty array.

After thought: Can we change the code and make the parameter nullable? It looks like it was meant to be nullable. What would that take?

New commit

[kocsismate]

My first thought: I think an empty array is an acceptable compromise. We can add a note in the manual about the unexpected behaviour with the default and temporary arguments but the signature can remain as an empty array.

IMO the stubs need to be an authoritative source of truth. Nikita even made tests which replaces default params with the ones in the stub: #5366

After thought: Can we change the code and make the parameter nullable? It looks like it was meant to be nullable. What would that take?

we didn't do this in case of #6933 for BC, although in case of PDOStatement::fetchObject() we would have more reason.

[nikic]

I'm missing some context here, what is the reason for this change?

[kocsismate]

I've just referenced the PR where the issue came up (generating PDO methodsynopses for the doc).

[Girgias]

IIRC all other cases which accept a constructor arguments (in the overloaded methods) accept null as a default, it might make more sense to change the implementation of the method (which is a one line change).

[nikic]

Okay, so why doesn't this simply change the implementation to make the parameter actually nullable?

[kocsismate]

Okay, so why doesn't this simply change the implementation to make the parameter actually nullable?

Due to the discussion in case of PDOStatement::fetch(), I didn't "dare" to change the signature. But I agree that it's the best course of action, and that's what I'd also prefer.

[nikic]

@kocsismate You wouldn't be changing the signature though. The signature is as declared in stubs, and that's the one that currently applies to inheriting code. There should be no BC impact from fixing the implementation.

[kocsismate]

You wouldn't be changing the signature though. The signature is as declared in stubs, and that's the one that currently applies to inheriting code. There should be no BC impact from fixing the implementation.

oh, you are right. I'm messing up things today...

[kamil-tekiela]

IIRC all other cases which accept a constructor arguments (in the overloaded methods) accept null as a default, it might make more sense to change the implementation of the method (which is a one line change).

It's more than one line of change due to the way this method is implemented. If you make it nullable you would get memory leaks as far as I can see.

[kocsismate]

It's more than one line of change due to the way this method is implemented. If you make it nullable you would get memory leaks as far as I can see.

I'll add a test for this, and I'll work on this later.

[kamil-tekiela]

@nikic Do we need to backport the memory leak fix to PHP 7.4?