Regression between RC1 and RC2?

Hi Hui,

-----Original Message-----
From: Xinchen Hui [mailto:laruence@php.net]
Sent: Sunday, October 9, 2016 8:06 AM
To: Anatol Belski anatol.php@belski.net
Cc: Davey Shafik davey@php.net; Nikita Popov nikita.ppv@gmail.com;
Derick Rethans derick@php.net; PHP Developers Mailing List
internals@lists.php.net
Subject: Re: [PHP-DEV] Regression between RC1 and RC2?

Hey:

On Sun, Oct 9, 2016 at 2:06 AM, Anatol Belski anatol.php@belski.net
wrote:

Hi,

-----Original Message-----
From: me@daveyshafik.com [mailto:me@daveyshafik.com] On Behalf Of
Davey Shafik
Sent: Friday, October 7, 2016 10:05 PM
To: Nikita Popov nikita.ppv@gmail.com
Cc: Derick Rethans derick@php.net; PHP Developers Mailing List
internals@lists.php.net
Subject: Re: [PHP-DEV] Regression between RC1 and RC2?

Yes, we should not mask the exception. The behavior in 7.0/7.1.0RC1
is
much
better IMO.

(As seen here: https://3v4l.org/EJpD4#v700)

• Davey

On Fri, Oct 7, 2016 at 12:52 PM, Nikita Popov nikita.ppv@gmail.com
wrote:

On Fri, Oct 7, 2016 at 9:31 PM, Derick Rethans derick@php.net
wrote:

Hi,

I was looking at Xdebug for PHP 7.1, and I ran into the
following
inconsistency:

https://3v4l.org/tHteN

I first thought that Xdebug was messing up, but it seems like
it's different behaviour in PHP itself. As I clearly return an
array from __debugInfo, I don't think the new result is the
correct one.

cheers,
Derick

This is due to https://github.com/php/php-src/commit/
2d8ab51576695630a7471ff829cc5ea10becdc0f, which landed in PHP-7.0
as
well.
The problem is that __debugInfo currently is not able to handle
exceptions gracefully. I think we should revert this change for
now as it hides the fact that the underlying cause of the error is
an
exception.

As far as I understand the bug #73067, it was about avoiding the fatal
error, not about avoiding the exception. Please correct if I'm wrong.
But given this, the fatal error still persists while the exception is
removed.
It looks like it's doing the exact reversed to what one would expect -
no fatal error and the exception can be catched.

I see that it's not yet released in 7.0, so I would prefer to revert
this in the release, at least. Hui, Nikita, do you think it's possible
to improve this for 7.0 in a follow up? I would revert in 7.0.12 and
there'll be room to fix it in the dev branch till 7.0.13. Otherwise
I'd suggest to revert to the previous behavior in 7.0+ and do a fix in
an appropriate higher branch.

The real problem is:

we are expecting __debugInfo always returns array,  but it doesn't if

exception is threw.

so, if we keep the exception,  then we need inserts checks for

exception (no

array) after every debugInfo is called, or, make a FATAL error like the
way I did

In the link sent from Derick https://3v4l.org/tHteN it's to see, that the
other behavior is affected. In that case, the exception is thrown from the
core, not from the user space, while __debuginfo() properly returns an
array. This is the behavior, that patch to #73067 indirectly affects.

however,  I think latter is better,  but maybe we could improve the

fatal error

message:

 " Fatal error,  __debugInfo must return an array, but an exception

with " msg"

is threw"

Yeah, that would be good, so the exception information is not lost. IMHO,
if we could get rid of the fatal error completely, it'd be more user
friendly. In that case the execution could continue, when exception is
catched. From the performance POV, it's anyway some additional effort,
either zend_clear_exception() or if(EG(exception)). But, if user chooses to
call __debuginfo(), it's clearly not performance oriented anyway.

If having exception instead of fatal is not viable for safety reasons (or
whatever), I'd take your suggestion to extend the fatal message with the
exception text. Since the exception is not catchable in that case anyway,
we'll at least keep the useful information.

Thanks

anatol

So the problem basically is that in PHP 7.0 both print_r and var_dump
directly print to output. This means that by the time the exception is
thrown, we've already written output. This makes it unclear how exactly the
exception should be handled. Is it okay to just print everything and handle
the exception afterward? This seems odd to me -- if an operation fails it
shouldn't do anything.

In PHP 7.1 I've rewritten print_r() to use an internal buffer, so we could
handle this case completely gracefully. The same change could be
implemented for var_dump(). With this approach we'd only print anything if
no exception occurred.

Nikita

Hi,

-----Original Message-----
From: Derick Rethans [mailto:derick@php.net]
Sent: Monday, October 10, 2016 5:26 PM
To: Xinchen Hui laruence@php.net
Cc: Anatol Belski anatol.php@belski.net; Davey Shafik davey@php.net;
Nikita Popov nikita.ppv@gmail.com; PHP Developers Mailing List
internals@lists.php.net
Subject: Re: [PHP-DEV] Regression between RC1 and RC2?

On Sun, Oct 9, 2016 at 2:06 AM, Anatol Belski anatol.php@belski.net
wrote:

From: me@daveyshafik.com [mailto:me@daveyshafik.com] On Behalf Of
Davey Shafik

Yes, we should not mask the exception. The behavior in
7.0/7.1.0RC1 is much better IMO.

(As seen here: https://3v4l.org/EJpD4#v700)

On Fri, Oct 7, 2016 at 12:52 PM, Nikita Popov nikita.ppv@gmail.com
wrote:

On Fri, Oct 7, 2016 at 9:31 PM, Derick Rethans derick@php.net
wrote:

I was looking at Xdebug for PHP 7.1, and I ran into the
following inconsistency:

https://3v4l.org/tHteN

I first thought that Xdebug was messing up, but it seems like
it's different behaviour in PHP itself. As I clearly return an
array from __debugInfo, I don't think the new result is the
correct one.

cheers, Derick

This is due to https://github.com/php/php-src/commit/
2d8ab51576695630a7471ff829cc5ea10becdc0f, which landed in
PHP-7.0 as well. The problem is that __debugInfo currently is
not able to handle exceptions gracefully. I think we should
revert this change for now as it hides the fact that the
underlying cause of the error is an exception.

As far as I understand the bug #73067, it was about avoiding the
fatal error, not about avoiding the exception. Please correct if I'm
wrong. But given this, the fatal error still persists while the
exception is removed. It looks like it's doing the exact reversed to
what one would expect - no fatal error and the exception can be
catched.

I see that it's not yet released in 7.0, so I would prefer to revert
this in the release, at least. Hui, Nikita, do you think it's
possible to improve this for 7.0 in a follow up? I would revert in
7.0.12 and there'll be room to fix it in the dev branch till 7.0.13.
Otherwise I'd suggest to revert to the previous behavior in 7.0+ and
do a fix in an appropriate higher branch.

The real problem is:

we are expecting __debugInfo always returns array,  but it doesn't

if exception is threw.

so, if we keep the exception,  then we need inserts checks for

exception (no array) after every debugInfo is called, or, make a FATAL
error like the way I did

however,  I think latter is better,  but maybe we could improve

the fatal error message:

Sorry, but that's making it a user problem, and not a technical problem
for us to
solve correctly. I very much expected an Uncaught Exception here.

Ok, for 7.0.12, I've reverted 2d8ab51576695630a7471ff829cc5ea10becdc0f.
Thus, there are yet two weeks till 7.0.13RC1 there to improve the patch.

Thanks

Anatol