Re: [PHP-DEV] Automatic implementation of Stringable may conflict with old, untyped arginfo declarations
On Wed, 8 Dec 2021 at 16:02, Jeremy Mikola wrote: > > > 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. -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] Automatic implementation of Stringable may conflict with old, untyped arginfo declarations
On Fri, Nov 12, 2021 at 7:00 AM Nikita Popov wrote: > > 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]( https://github.com/mongodb/mongo-php-driver/pull/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]( https://github.com/php/php-src/commit/86379b6710f972e0d4a11c89ce28d5768d9824d3). 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
Re: [PHP-DEV] Automatic implementation of Stringable may conflict with old, untyped arginfo declarations
On Wed, Nov 10, 2021 at 10:13 PM Jeremy Mikola wrote: > > > On Tue, Nov 9, 2021 at 4:30 AM Nikita Popov wrote: > >> >> 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
Re: [PHP-DEV] Automatic implementation of Stringable may conflict with old, untyped arginfo declarations
On Tue, Nov 9, 2021 at 4:30 AM Nikita Popov wrote: > > 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
Re: [PHP-DEV] Automatic implementation of Stringable may conflict with old, untyped arginfo declarations
On Tue, Nov 9, 2021 at 2:11 AM Jeremy Mikola wrote:
>
> 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.
>
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
[PHP-DEV] Automatic implementation of Stringable may conflict with old, untyped arginfo declarations
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
