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
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
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
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.
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
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.
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!
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.
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
Hey Nikita, hey all
Am 13.05.19 um 16:05 schrieb 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,
NikitaIf 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
,,,
(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 |
+---------------------------------------------------------------------+
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,
NikitaIf 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
Hi Nikita,
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?
Matteo Beccati
Development & Consulting - http://www.beccati.com/
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
Hi Christoph,
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.
Matteo Beccati
Development & Consulting - http://www.beccati.com/
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
Hi Dan,
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.
Matteo Beccati
Development & Consulting - http://www.beccati.com/
Hi Dan,
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
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" ;)
Matteo Beccati
Development & Consulting - http://www.beccati.com/