[RFC] Allow throwing exceptions from __toString()

  105515
April 30, 2019 13:32 nikita.ppv@gmail.com (Nikita Popov)
Hi internals,

I've already brought up this topic on list a couple of weeks ago... I've
now updated the implementation based on feedback at the time, and turned
this into a proper RFC:

https://wiki.php.net/rfc/tostring_exceptions

Regards,
Nikita
  105516
April 30, 2019 14:13 Danack@basereality.com (Dan Ackroyd)
On Tue, 30 Apr 2019 at 14:33, Nikita Popov ppv@gmail.com> wrote:
> > Hi internals, > > I've already brought up this topic on list a couple of weeks ago... I've > now updated the implementation based on feedback at the time, and turned > this into a proper RFC:
This RFC is probably beyond the capabilities of most people to understand the potential downsides. Please could someone anyone who understands the implications for what this means for the internals of the PHP engine provide some detailed reasons for why this shouldn't be approved, if there are any reasons. Otherwise voters might 'irresponsibly' vote yes, without being aware of them. cheers Dan Ack
  105525
April 30, 2019 18:49 smalyshev@gmail.com (Stanislav Malyshev)
Hi!

> Please could someone anyone who understands the implications for what > this means for the internals of the PHP engine provide some detailed > reasons for why this shouldn't be approved, if there are any reasons. > > Otherwise voters might 'irresponsibly' vote yes, without being aware of them.
This is the tricky one because it gets into one of the corner cases of the engine that has never worked perfectly. We have a bunch of places where we can get some code executed in the middle of other code executed, and it never worked particularly well. One of such places is (un)serialization, another is error and exception handling. Generally, running code from the guts of the engine is dangerous, because there might be stale pointers, race conditions (not the parallel kind but the kind "we checked it above, but below it may no longer be the case because we run code in between"), shared mutable state, etc. and it's nigh impossible to ensure nothing gets broken by it. That said, the work done on the patch is super impressive. I am reasonably sure that Nikita caught as many instances of something going wrong when string conversion fails as humanly possible. And I agree that banning exceptions didn't actually solve the problem anyway, because error handlers present the same exactly issue - exceptions are just much more prominent and immediate in their effects, but with some hostile coding - or even some inadvertently convoluted one - you'd run into the same issues again. Some extensions will probably have some trouble - though with high likelyhood they already do anyway. I think since the reason why it was banned is largely gone with this patch it's a good step forward. There are probably still some corner cases that would not work properly - I am especially worried about variables left un-initialized in places where they are expected to be initialized - like internal classes - but I think the awesome job done on this patch is a good step forward and unless we discover some critical issue blocking it, it makes sense to do it. I plan to try and review the patch in my copious free time, but so far I do not have any objections beyond "yet another place where things could go wrong". Since that place effectively already existed, I think this RFC is good. -- Stas Malyshev smalyshev@gmail.com
  105539
May 1, 2019 02:53 bishop@php.net (Bishop Bettini)
On Tue, Apr 30, 2019 at 2:49 PM Stanislav Malyshev <smalyshev@gmail.com>
wrote:

> > That said, the work done on the patch is super impressive. I am > reasonably sure that Nikita caught as many instances of something going > wrong when string conversion fails as humanly possible. And I agree that > banning exceptions didn't actually solve the problem anyway, because > error handlers present the same exactly issue - exceptions are just much > more prominent and immediate in their effects, but with some hostile > coding - or even some inadvertently convoluted one - you'd run into the > same issues again. Some extensions will probably have some trouble - > though with high likelyhood they already do anyway.
I second that "super impressive work". I'm certain some of why it's been deferred is because doing it was, well, unpleasant. Thanks Nikita for wrestling this. I spent an hour today going through the patch, especially its effect on the phar and imap extensions, and Zend/*. The patch seems good to me, minus a few targeted comments. But I'd still think this would be a "many eyes needed" kind of PR, especially from extension maintainers. +1 from me.
  105544
May 1, 2019 11:36 Danack@basereality.com (Dan Ackroyd)
On Wed, 1 May 2019 at 03:54, Bishop Bettini <bishop@php.net> wrote:
> > But I'd still think this would be a "many eyes needed" kind of PR, especially from extension maintainers.
Hypothetically, what should these extension maintainers be looking for? Other than "Mmm-hmm. Mmm-HMM. I know some of these words!" ? cheers Dan Ack
  105546
May 1, 2019 14:52 bishop@php.net (Bishop Bettini)
On Wed, May 1, 2019 at 7:36 AM Dan Ackroyd <Danack@basereality.com> wrote:

> On Wed, 1 May 2019 at 03:54, Bishop Bettini <bishop@php.net> wrote: > > > > But I'd still think this would be a "many eyes needed" kind of PR, > especially from extension maintainers. > > Hypothetically, what should these extension maintainers be looking for? > > Other than "Mmm-hmm. Mmm-HMM. I know some of these words!" ? >
Indeed. I'm by no means an expert, but here's what I did for phar and imap extensions. I read the RFC and "got to know" the new helper methods for resolving stringy content. I then looked at the PR for all changes to phar and imap, verifying that I understood the changes Nikita made. Then I went to master and PHP-7.4 branches and looked for any other cases that might need to be changed, and while there took mental note of improvement opportunities should this RFC pass. When I was in there, I was basically looking for "convert_to_string" (and friends) and if I found any that Nikita hadn't already found, replacing those with one of the new helpers and making sure I understood the choice Nikita made and commenting if I disagreed. I was also looking for any case where the conversion touched data that persisted and made sure it wasn't going to get trashed by a bad conversion. I also went through code outside phar and imap looking for things I didn't understand or surprised me or didn't look correct. Suffice to say, there's a lot of code to look through. Some code surprised me, like that the (new DateInterval(...))->{$badStr} unhappy path didn't adhere to the has_property contract, so that was a bugfix Nikita made as an artifact of this sweep, which is great and I think emblematic of the reason to get more eyes on this: might be more of those lurking in the code base.
  105547
May 1, 2019 16:08 mo.mu.wss@gmail.com ("M. W. Moe")
Hello,

the _convert_to_string return signature should be changed i.e returning a
status;
hence, accordingly to this status, within a context caller, you may decide
to trigger
an exception  or not ; that's not the role of a conversion function to
handle
those concerns; thus the realm is wider than what it is sold here.

Cheers!

On Wed, May 1, 2019 at 7:53 AM Bishop Bettini <bishop@php.net> wrote:

> On Wed, May 1, 2019 at 7:36 AM Dan Ackroyd <Danack@basereality.com> wrote: > > > On Wed, 1 May 2019 at 03:54, Bishop Bettini <bishop@php.net> wrote: > > > > > > But I'd still think this would be a "many eyes needed" kind of PR, > > especially from extension maintainers. > > > > Hypothetically, what should these extension maintainers be looking for? > > > > Other than "Mmm-hmm. Mmm-HMM. I know some of these words!" ? > > > > Indeed. I'm by no means an expert, but here's what I did for phar and imap > extensions. I read the RFC and "got to know" the new helper methods for > resolving stringy content. I then looked at the PR for all changes to phar > and imap, verifying that I understood the changes Nikita made. Then I went > to master and PHP-7.4 branches and looked for any other cases that might > need to be changed, and while there took mental note of improvement > opportunities should this RFC pass. > > When I was in there, I was basically looking for "convert_to_string" (and > friends) and if I found any that Nikita hadn't already found, replacing > those with one of the new helpers and making sure I understood the choice > Nikita made and commenting if I disagreed. I was also looking for any case > where the conversion touched data that persisted and made sure it wasn't > going to get trashed by a bad conversion. > > I also went through code outside phar and imap looking for things I didn't > understand or surprised me or didn't look correct. Suffice to say, there's > a lot of code to look through. Some code surprised me, like that the (new > DateInterval(...))->{$badStr} unhappy path didn't adhere to the > has_property contract, so that was a bugfix Nikita made as an artifact of > this sweep, which is great and I think emblematic of the reason to get more > eyes on this: might be more of those lurking in the code base. >
  105687
May 13, 2019 14:05 nikita.ppv@gmail.com (Nikita Popov)
On Tue, Apr 30, 2019 at 3:32 PM Nikita Popov ppv@gmail.com> wrote:

> Hi internals, > > I've already brought up this topic on list a couple of weeks ago... I've > now updated the implementation based on feedback at the time, and turned > this into a proper RFC: > > https://wiki.php.net/rfc/tostring_exceptions > > Regards, > Nikita >
If there's no more comments here, I'd like to start voting on this RFC tomorrow. Nikita
  105689
May 13, 2019 15:08 Andreas Heigl <andreas@heigl.org>
--9Dqk0NxqBIJ3upBbTR3EME1kpmg7jrQAM
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable

Hey Nikita, hey all

Am 13.05.19 um 16:05 schrieb Nikita Popov:
> On Tue, Apr 30, 2019 at 3:32 PM Nikita Popov ppv@gmail.com> wro= te:
>=20 >> Hi internals, >> >> I've already brought up this topic on list a couple of weeks ago... I'= ve
>> now updated the implementation based on feedback at the time, and turn= ed
>> this into a proper RFC: >> >> https://wiki.php.net/rfc/tostring_exceptions >> >> Regards, >> Nikita >> >=20 > If there's no more comments here, I'd like to start voting on this RFC > tomorrow.
As the change is technically a BC-break (as you state in the RFC) I wondered why the target is PHP7.4 and not 8. So far I didn't find a discussion on that but I might not have dug deep enough. So if this has already been discussed then I'd really appreciate a hint to the discussion. Thanks Andreas --=20 ,,, (o o) +---------------------------------------------------------ooO-(_)-Ooo-+ | Andreas Heigl | | mailto:andreas@heigl.org N 50=C2=B022'59.5" E 08=C2=B0= 23'58" | | http://andreas.heigl.org http://hei.gl/wiFKy7 | +---------------------------------------------------------------------+ | http://hei.gl/root-ca | +---------------------------------------------------------------------+ --9Dqk0NxqBIJ3upBbTR3EME1kpmg7jrQAM--
  105695
May 14, 2019 10:44 nikita.ppv@gmail.com (Nikita Popov)
On Mon, May 13, 2019 at 5:08 PM Andreas Heigl <andreas@heigl.org> wrote:

> Hey Nikita, hey all > > Am 13.05.19 um 16:05 schrieb Nikita Popov: > > On Tue, Apr 30, 2019 at 3:32 PM Nikita Popov ppv@gmail.com> > wrote: > > > >> Hi internals, > >> > >> I've already brought up this topic on list a couple of weeks ago... I've > >> now updated the implementation based on feedback at the time, and turned > >> this into a proper RFC: > >> > >> https://wiki.php.net/rfc/tostring_exceptions > >> > >> Regards, > >> Nikita > >> > > > > If there's no more comments here, I'd like to start voting on this RFC > > tomorrow. > > As the change is technically a BC-break (as you state in the RFC) I > wondered why the target is PHP7.4 and not 8. > > So far I didn't find a discussion on that but I might not have dug deep > enough. So if this has already been discussed then I'd really appreciate > a hint to the discussion. >
The target is 7.4 because the BC break is minimal and largely theoretical. I think the main affected code will be Symfony's existing hack around the issue -- they'll have to disable the hack for PHP >= 7.4 and just use the native support. Nikita
  105699
May 14, 2019 13:52 php@beccati.com (Matteo Beccati)
Hi Nikita,

On 14/05/2019 12:44, Nikita Popov wrote:
> The target is 7.4 because the BC break is minimal and largely theoretical. > I think the main affected code will be Symfony's existing hack around the > issue -- they'll have to disable the hack for PHP >= 7.4 and just use the > native support.
How would the change affect extension developers? Cheers -- Matteo Beccati Development & Consulting - http://www.beccati.com/
  105700
May 14, 2019 14:13 cmbecker69@gmx.de ("Christoph M. Becker")
On 14.05.2019 at 15:52, Matteo Beccati wrote:

> On 14/05/2019 12:44, Nikita Popov wrote: > >> The target is 7.4 because the BC break is minimal and largely >> theoretical. >> I think the main affected code will be Symfony's existing hack around the >> issue -- they'll have to disable the hack for PHP >= 7.4 and just use the >> native support. > > How would the change affect extension developers?
See <https://wiki.php.net/rfc/tostring_exceptions#extension_guidelines>. -- Christoph M. Becker
  105705
May 14, 2019 16:55 php@beccati.com (Matteo Beccati)
Hi Christoph,

On 14/05/2019 16:13, Christoph M. Becker wrote:
> On 14.05.2019 at 15:52, Matteo Beccati wrote: >> How would the change affect extension developers? > > See <https://wiki.php.net/rfc/tostring_exceptions#extension_guidelines>.
Perhaps the section can be expanded to illustrate to non-internal people how much work it is to upgrade the average extension or what happens if they don't. Cheers -- Matteo Beccati Development & Consulting - http://www.beccati.com/
  105709
May 15, 2019 01:15 Danack@basereality.com (Dan Ackroyd)
On Tue, 14 May 2019 at 17:55, Matteo Beccati <php@beccati.com> wrote:
> > Perhaps the section can be expanded to illustrate to non-internal people > how much work it is to upgrade the average extension or what happens if > they don't. >
It will vary per extension. For some it will be none. For others it will be 'some' time for each place the extension converts a PHP value to a string. Maybe a couple of hours for the first one, less time after that.
> what happens if they don't.
Immediately nothing. Eventually, when a __toString() method throws an exception, and the extension doesn't check for an exception being throw, bad things will happen. But in my opinion, if people don't understand the implications of the RFC even after reading the details in the RFC and this thread, then this probably is an RFC they should skip voting on. cheers Dan Ack
  105718
May 15, 2019 17:55 php@beccati.com (Matteo Beccati)
Hi Dan,

On 15/05/2019 03:15, Dan Ackroyd wrote:
> Immediately nothing. > > Eventually, when a __toString() method throws an exception, and the > extension doesn't check for an exception being throw, bad things will > happen. > > But in my opinion, if people don't understand the implications of the > RFC even after reading the details in the RFC and this thread, then > this probably is an RFC they should skip voting on.
I don't agree. I've worked on a few bug fixes and features in php-src, but never worked directly with exceptions, so I have to admit my ignorance on the topic. I was expecting "bad things to happen", but I wasn't quite sure about it. "Extension authors who would like to ensure that they handle exceptions from string conversions gracefully" and "It is generally sufficient to check whether an exception has been thrown" do not convey any sense of urgency. Since there are many people who can vote and only do PHP userland, I believe that clarifying impact on the RFC itself would surely benefit everyone. Suggesting that they shouldn't vote is definitely not helping. My 2c. Cheers -- Matteo Beccati Development & Consulting - http://www.beccati.com/
  105719
May 15, 2019 19:00 nikita.ppv@gmail.com (Nikita Popov)
On Wed, May 15, 2019 at 7:55 PM Matteo Beccati <php@beccati.com> wrote:

> Hi Dan, > > On 15/05/2019 03:15, Dan Ackroyd wrote: > > Immediately nothing. > > > > Eventually, when a __toString() method throws an exception, and the > > extension doesn't check for an exception being throw, bad things will > > happen. > > > > But in my opinion, if people don't understand the implications of the > > RFC even after reading the details in the RFC and this thread, then > > this probably is an RFC they should skip voting on. > > I don't agree. I've worked on a few bug fixes and features in php-src, > but never worked directly with exceptions, so I have to admit my > ignorance on the topic. I was expecting "bad things to happen", but I > wasn't quite sure about it. > > "Extension authors who would like to ensure that they handle exceptions > from string conversions gracefully" and "It is generally sufficient to > check whether an exception has been thrown" do not convey any sense of > urgency. > > Since there are many people who can vote and only do PHP userland, I > believe that clarifying impact on the RFC itself would surely benefit > everyone. Suggesting that they shouldn't vote is definitely not helping. >
Quoting from the RFC:
> While checking every single string conversion certainly puts you on the safe side, leaving out these checks will usually only result in some
unneeded computation and possibly redundant warnings. The main thing you should watch out for are operations modifying persistent structures such as databases. Not explicitly handling exceptions from __toString() will not cause segfaults or memory corruption if you don't do anything. It only means that your function call will not abort at the earliest possible opportunity. If you have functions modifying persistent datastores it's a good idea to review them to make sure you handle exceptions right away, but otherwise I wouldn't be overly concerned. Nikita
  105720
May 15, 2019 19:46 php@beccati.com (Matteo Beccati)
On 15/05/2019 21:00, Nikita Popov wrote:
>> While checking every single string conversion certainly puts you on the > safe side, leaving out these checks will usually only result in some > unneeded computation and possibly redundant warnings. The main thing you > should watch out for are operations modifying persistent structures such as > databases. > > Not explicitly handling exceptions from __toString() will not cause > segfaults or memory corruption if you don't do anything. It only means that > your function call will not abort at the earliest possible opportunity. If > you have functions modifying persistent datastores it's a good idea to > review them to make sure you handle exceptions right away, but otherwise I > wouldn't be overly concerned.
Thanks Nikita that's much clearer to me now. I'm not as concerned as this is much different from "bad things will happen" ;) Cheers -- Matteo Beccati Development & Consulting - http://www.beccati.com/