Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:126215
Date: Tue, 21 Jan 2025 05:39:26 +0000
To: PHP internals <internals@lists.php.net>
Subject: [PHP-DEV] Use usual semantics for PDO::FETCH_CLASS constructor arguments
Message-ID: <rAlEx_hYFBFbQdkaMGhgr0pyjY6nU4tGMlrmm7hD68vJnkHPYaNRq0PBP-mpkVNUFd08UCX8FX_s55TqiEMr3WTjJNp8xGFTglS-JWETM-M=@gpb.moe>
Feedback-ID: 96993444:user:proton
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From: internals@gpb.moe ("Gina P. Banyard")

Hello internals,

For the past few weeks I have been refactoring the PDO extension to reduce =
the complexity of the implementation but also lower the memory requirement =
of the _pdo_stmt_t struct.
And in this process I rediscovered an oddity which prevents removing the us=
age of the FCI struct (which takes 64 bytes) from it.

The constructor arguments for the class are passed as a PHP array to PDOSta=
tement::setFetchMode(), PDOStatement::fetchAll(), and PDOStatment::fetchObj=
ect().
However, this array of parameters does not have the same semantics as the $=
args parameter of call_user_func_array(). (CUFA for short.)
The reason for this is that it uses a rather unintuitive and obscure Zend A=
PI called zend_fcall_info_args_ex(),
this API sets the arguments of a call by copying each argument one after th=
e other as a positional argument.
And if a zend_function pointer is provided, it will check if the function a=
rgument should be passed by reference and automatically wrap values into a =
reference without a warning.
This behaviour is at odds with passing a keyed array to CUFA, where the key=
 will act as a named argument look-up, and with normal calling semantics wh=
ere a by-ref argument being passed by value will trigger an E_WARNING.

Moreover, this API is effectively unused. A SourceGraph search [1] came up =
with 9 results, where only 2 of them are relevant:
- The UOPZ extension
- The Zephir project, which allows generating a PHP C extension from PHPesq=
ue code
- All other 7 projects are extensions generated by using Zephir

I have submitted PRs to Zephir [2] and UOPZ [3] that have been merged that =
remove the usage of this API by instead passing the HashTable pointer of th=
e array as the named_params field of the FCI.
Thus, the only remaining usage of this API I can find is now in PDO.
This conversion of using the named_params field of the FCI for calling the =
constructor can also be done for PDO,
allowing the removal of the FCI from _pdo_stmt_t, convert the ctor_arg zval=
 (which takes 16 bytes) to a HashTable* (taking only 8 bytes), and remove v=
arious copies of the constructor arguments.
Doing this causes some minor BC breaks as this aligns the behaviour with CU=
FA which can be seen in the latest PR implementation. [4]

It is possible to emulate the previous behaviour, but this requires doing a=
 deep copy of the constructor arguments, negating that benefit.

To me, these minor BC breaks are acceptable as:

- by-ref arguments in a constructor seems already rare, even more so in an =
object that represents a database row
- having an argument array behave differently than CUFA or what you would r=
eceive with variadic arugments is somewhat a language inconsistency.

Thoughts?

For reference, this is effectively the same point I raised in a previous in=
ternals thread, which didn't really get much attention. [5]

Best regards,

Gina P. Banyard

[1] https://sourcegraph.com/search?q=3Dcontext:global+lang:C+zend_fcall_inf=
o_args_ex+-file:zend_API.h+-file:zend_API.c+-file:pdo_stmt.c&patternType=3D=
keyword&sm=3D0
[2] Zephir PR: https://github.com/zephir-lang/zephir/pull/2443
[3] UOPZ PR: https://github.com/krakjoe/uopz/pull/188
[4] https://github.com/php/php-src/pull/17427
[5] https://externals.io/message/118847