On Sun, Oct 9, 2016 at 1:49 PM, Anatol Belski <anatol....@belski.net> wrote:
> Hi Hui, > > > -----Original Message----- > > From: Xinchen Hui [mailto:larue...@php.net] > > Sent: Sunday, October 9, 2016 8:06 AM > > To: Anatol Belski <anatol....@belski.net> > > Cc: Davey Shafik <da...@php.net>; Nikita Popov <nikita....@gmail.com>; > > Derick Rethans <der...@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....@belski.net> > wrote: > > > > > Hi, > > > > > > > -----Original Message----- > > > > From: m...@daveyshafik.com [mailto:m...@daveyshafik.com] On Behalf Of > > > > Davey Shafik > > > > Sent: Friday, October 7, 2016 10:05 PM > > > > To: Nikita Popov <nikita....@gmail.com> > > > > Cc: Derick Rethans <der...@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....@gmail.com> > > > wrote: > > > > > > > > > On Fri, Oct 7, 2016 at 9:31 PM, Derick Rethans <der...@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
