Everything

php

[RFC] Allow throwing exceptions from __toString()

php.internals

Nikita Popov

7 years ago

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

Dan Ackroyd

7 years ago

On Tue, 30 Apr 2019 at 14:33, Nikita Popov <nikita.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

Stas Malyshev

7 years ago

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

Bishop Bettini

7 years ago

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.

Dan Ackroyd

7 years ago

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

Bishop Bettini

7 years ago

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.

M. W. Moe

7 years ago

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:

Nikita Popov

7 years ago

On Tue, Apr 30, 2019 at 3:32 PM Nikita Popov <nikita.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

Andreas Heigl

7 years ago

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 <nikita.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.

Thanks

Andreas

--
,,,
(o o)
+---------------------------------------------------------ooO-(_)-Ooo-+
| Andreas Heigl                                                       |
| mailto:andreas@heigl.org                  N 50°22'59.5" E 08°23'58" |
| http://andreas.heigl.org                       http://hei.gl/wiFKy7 |
+---------------------------------------------------------------------+
| http://hei.gl/root-ca                                               |
+---------------------------------------------------------------------+

Nikita Popov

7 years ago

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 <nikita.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

Matteo Beccati

7 years ago

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/

Christoph Becker

7 years ago

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

Matteo Beccati

7 years ago

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/

Dan Ackroyd

7 years ago

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

Matteo Beccati

7 years ago

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/

Nikita Popov

7 years ago

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