https://github.com/php/php-src/commit/b302bfabe7fadd07b022ee30aacf7f41ab1ae0fa
recently added automatic implementation of Stringable (
https://wiki.php.net/rfc/stringable) for internal classes. This introduced
fatal errors for each existing __toString() in ext-mongodb:
Declaration of MongoDB\BSON\BinaryInterface::__toString() must be
compatible with Stringable::__toString(): string in Unknown on line 0
Declaration of MongoDB\BSON\Binary::__toString() must be compatible with
Stringable::__toString(): string in Unknown on line 0
...
Our arginfos for these methods have historically never reported a return
type, going back to when it was originally written for PHP 5.x. For example:
ZEND_BEGIN_ARG_INFO_EX(ai_BinaryInterface_void, 0, 0, 0)
ZEND_END_ARG_INFO()
static zend_function_entry php_phongo_binary_interface_me[] = {
ZEND_ABSTRACT_ME(BinaryInterface, __toString, ai_BinaryInterface_void)
// ...
I noted that _ZendTestClass (referenced in the original commit's tests)
uses ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX to declare a string return
type (
https://github.com/php/php-src/blob/eda9d7ac22c70f75a44bf33139acf8c812d69944/ext/zend_test/test_arginfo.h#L74
).
While that's a trivial change we can make in ext-mongodb, I wonder if this
may have been an unanticipated BC break for third-party extensions. I
imagine ext-mongodb is not the only extension with older arginfo
declarations predating the introduction of type reporting in later PHP
versions.
--
jeremy mikola
https://github.com/php/php-src/commit/b302bfabe7fadd07b022ee30aacf7f41ab1ae0fa
recently added automatic implementation of Stringable (
https://wiki.php.net/rfc/stringable) for internal classes. This introduced
fatal errors for each existing __toString() in ext-mongodb:Declaration of MongoDB\BSON\BinaryInterface::__toString() must be compatible with Stringable::__toString(): string in Unknown on line 0 Declaration of MongoDB\BSON\Binary::__toString() must be compatible with Stringable::__toString(): string in Unknown on line 0 ...Our arginfos for these methods have historically never reported a return
type, going back to when it was originally written for PHP 5.x. For
example:ZEND_BEGIN_ARG_INFO_EX(ai_BinaryInterface_void, 0, 0, 0) ZEND_END_ARG_INFO() static zend_function_entry php_phongo_binary_interface_me[] = { ZEND_ABSTRACT_ME(BinaryInterface, __toString, ai_BinaryInterface_void) // ...I noted that _ZendTestClass (referenced in the original commit's tests)
uses ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX to declare a string return
type (While that's a trivial change we can make in ext-mongodb, I wonder if this
may have been an unanticipated BC break for third-party extensions. I
imagine ext-mongodb is not the only extension with older arginfo
declarations predating the introduction of type reporting in later PHP
versions.
Thanks for pointing this out!
In
https://github.com/php/php-src/commit/a551b083073ea08f8fc53b0e1a6380b6de26cc6b
I've added a hack to add the string return type if it is missing and thus
make the signature compatible with the interface. That should address the
immediate compatibility issue.
In
https://github.com/php/php-src/commit/86379b6710f972e0d4a11c89ce28d5768d9824d3
I have added a warning if this happens. The warning is only for master
(i.e. PHP 8.2) to make sure that extensions add the type eventually, and we
can drop this workaround.
Regards,
Nikita
In
https://github.com/php/php-src/commit/a551b083073ea08f8fc53b0e1a6380b6de26cc6b
I've added a hack to add the string return type if it is missing and thus
make the signature compatible with the interface. That should address the
immediate compatibility issue.
Thanks for the quick fix.
A related question that came up, and is most likely unique to ext-mongodb,
follows. Many of our classes with toString() methods also implement a
corresponding interface with a toString() method. For example:
- https://www.php.net/manual/en/class.mongodb-bson-binary.php
- https://www.php.net/manual/en/class.mongodb-bson-binaryinterface.php
I'm in the process of adding explicit return type info to all of our
toString() arginfos (classes and interfaces), but a thought occurred to me
that doing so may be a subtle BC break for userland classes implementing
these interfaces, since an explicit string return type would then become
necessary. But if I only modify our classes and leave our interfaces as-is,
PHP rightfully reports an error because the class' toString() method cannot
conform to both Stringable (with type info) and our interface (without type
info) -- at least PHP versions before 8.1.0RC6.
Reading the patch above, it looks like the automatic Stringable
implementation only kicks in when the arginfo has no existing return type
info. In that case, I think the only option to completely avoid a BC break
would be to continue to leave our return type info omitted (on both our
classes and interfaces) and allow PHP 8.1+ to apply it automatically. Is
that correct?
--
jeremy mikola
In
https://github.com/php/php-src/commit/a551b083073ea08f8fc53b0e1a6380b6de26cc6b
I've added a hack to add the string return type if it is missing and thus
make the signature compatible with the interface. That should address the
immediate compatibility issue.Thanks for the quick fix.
A related question that came up, and is most likely unique to ext-mongodb,
follows. Many of our classes with toString() methods also implement a
corresponding interface with a toString() method. For example:
- https://www.php.net/manual/en/class.mongodb-bson-binary.php
- https://www.php.net/manual/en/class.mongodb-bson-binaryinterface.php
I'm in the process of adding explicit return type info to all of our
toString() arginfos (classes and interfaces), but a thought occurred to me
that doing so may be a subtle BC break for userland classes implementing
these interfaces, since an explicit string return type would then become
necessary. But if I only modify our classes and leave our interfaces as-is,
PHP rightfully reports an error because the class' toString() method cannot
conform to both Stringable (with type info) and our interface (without type
info) -- at least PHP versions before 8.1.0RC6.Reading the patch above, it looks like the automatic Stringable
implementation only kicks in when the arginfo has no existing return type
info. In that case, I think the only option to completely avoid a BC break
would be to continue to leave our return type info omitted (on both our
classes and interfaces) and allow PHP 8.1+ to apply it automatically. Is
that correct?
With the introduction of Stringable PHP also started automatically adding
the string result type to __toString(), specifically for compatibility with
the interface. As such, it should be safe to add the string return type
everywhere for PHP >= 8.0. (If you use stubs with legacy arginfo, that
would automatically give you the right behavior: types on PHP 8, no types
on old versions.)
Regards,
Nikita
With the introduction of Stringable PHP also started automatically adding
the string result type to __toString(), specifically for compatibility with
the interface. As such, it should be safe to add the string return type
everywhere for PHP >= 8.0. (If you use stubs with legacy arginfo, that
would automatically give you the right behavior: types on PHP 8, no types
on old versions.)
We haven't explored using stubs, but I'll keep that in mind. Andreas has
definitely mentioned that in the past and I believe it's on the list of
ideas to implement once we revisit typing across the board for the
extension (which we plan to do since we finally dropped support for PHP
7.1).
I previously added explicit string return types to all of our toString
methods (including interfaces), which I understand is a small BC break for
any userland classes implementing those interfaces (as rare as that is).
Just yesterday, I had an idea to use tentative return type info on the
interface toString methods (mongodb/mongo-php-driver#1283), which seems to
require no changes at all to userland classes. ReturnTypeWillChange
attributes are not even required on PHP 8.1+ since the Stringable behavior
will automatically add the return type info if it's omitted. For PHP 8.0
and earlier, we map the ZEND_BEGIN_ARG_WITH_TENTATIVE_RETURN_TYPE_INFO_EX
macro to ZEND_BEGIN_ARG_INFO_EX -- so these methods will continue to have
no return types there (also avoiding a BC break).
I tested the change (i.e. tentative return type info for interfaces) with
PHP's master branch to cover the warning you added in 86379b6.
There seems to be no conflict, which seems reasonable considering that
return type is only "tentative" from the context of userland (and only
until PHP 9.0, when it becomes enforced). Tentative return types are still
considered the actual return type for internal purposes and satisfies the
check for Stringable compatibility.
Are we likely to run into issues with this approach? If so, I'll consider
your last suggestion to define non-tentative return type info for
interfaces on PHP 8.0+ (where Stringable will add it to userland classes
automatically), and leave return type info absent for PHP 7.x. But if both
are viable solutions, I think I prefer the tentative return type info
approach.
--
jeremy mikola
We haven't explored using stubs, but I'll keep that in mind. ...
(which we plan to do since we finally dropped support for PHP
7.1).
FYI, the stub files maintain #ifdef info through their processing,
which allows some useful shenanigans* e.g.:
https://github.com/Imagick/imagick/blob/86d95fed787d60db110ccfeb2d89ec53f1d1f5f7/Imagick.stub.php#L168-L182
https://github.com/Imagick/imagick/blob/86d95fed787d60db110ccfeb2d89ec53f1d1f5f7/Imagick_arginfo.h#L748-L778
So the stubs are used to generate arg info for Imagick from PHP 5.4 to
8.1, across lots of versions of ImagickMagick.
I'm finding managing function signatures in a PHP file, in standard
PHP format, is a lot nicer than maintaing that info in C files.
cheers
Dan
Ack
- Though, tbh, I wished I'd known about /** @generate-legacy-arginfo
*/ before doing some of the hacks in the Imagick regen_arginfo.sh
script.