Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:116309
MIME-Version: 1.0
References: <CANzZr6kgZCF4DRKOnMTQr5gZ1LRQ2mMFn1OottwQR87qL=t25g@mail.gmail.com>
In-Reply-To: <CANzZr6kgZCF4DRKOnMTQr5gZ1LRQ2mMFn1OottwQR87qL=t25g@mail.gmail.com>
Date: Tue, 9 Nov 2021 10:29:58 +0100
Message-ID: <CAF+90c9i90FLNaL=bvN4=L0iJbb4RxGrfU-vt7Q09ZvG+gYsjA@mail.gmail.com>
To: Jeremy Mikola <jmikola@gmail.com>
Cc: PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000bb9d9705d057c169"
Subject: Re: [PHP-DEV] Automatic implementation of Stringable may conflict
 with old, untyped arginfo declarations
From: nikita.ppv@gmail.com (Nikita Popov)

--000000000000bb9d9705d057c169
Content-Type: text/plain; charset="UTF-8"

On Tue, Nov 9, 2021 at 2:11 AM Jeremy Mikola <jmikola@gmail.com> 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

--000000000000bb9d9705d057c169--