Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:93485
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain mindplay.dk from 209.85.218.53 cause and error)
MIME-Version: 1.0
In-Reply-To: <CAMUwpuR=ES4eWTofrAcfC434nFQOrAyp5cy-wVdAykVr3BwE-A@mail.gmail.com>
References: <CADqTB_jC-EZ5p+c=q0dmRgaYAkyRVbdNFNUQRGhfaS7GaOTR9g@mail.gmail.com>
	<72f547d6-5fcf-3cd2-960d-cb612a429e47@php.net>
	<CANUQDCiEDeCV3RAr0bg+=-PHPDYkk-cP-ih4TnTGUzz7SNakMw@mail.gmail.com>
	<CADqTB_j67-ocNaWm5sRHbC4vJF2t7WBWUqrEXu9-Kk_MSi4CgA@mail.gmail.com>
	<CAF1yvAQozS1qpCbBNdAJu4gT5LbOEUimv2W-nWtkaUTbVZFubA@mail.gmail.com>
	<CANUQDCjnEgJwT5zEb50sWA5=OeF-LRzPDu_1+-E70as2N10oXw@mail.gmail.com>
	<fb93f245-a5e6-53f7-9fee-deaebb847b78@gmail.com>
	<CADqTB_inxTXkg+mVGyBVsP1drDqsWfaLMKe00R=veBQT7ZKedQ@mail.gmail.com>
	<CAMUwpuR=ES4eWTofrAcfC434nFQOrAyp5cy-wVdAykVr3BwE-A@mail.gmail.com>
Date: Tue, 24 May 2016 17:32:22 +0200
Message-ID: <CADqTB_jXnLZx-F_-08+F1APoUTd_9C-Gt+vuc2GAk=xWeXFw=A@mail.gmail.com>
To: Julien Pauli <jpauli@php.net>
Cc: Rowan Collins <rowan.collins@gmail.com>, PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] Exception::getLine()
From: rasmus@mindplay.dk (Rasmus Schultz)

> it would be more logical to collect the trace in the ZEND_THROW Opcode instead of in the create_handler of the Exception class

I've been pondering a means of doing this with relatively few BC issues.

There are four use-cases right now:

1. Direct: throw new Exception()
2. Indirect: throw $factory->createException()
3. Re-throwing: throw $exception
4. Not throwing: $exception = new Exception()

The first use-case (throw new) makes up the large majority, and would
be unaffected by this change.

The second use-case (indirect throw, factory) and third use-case
(re-throwing) are both broken as-is - in either case, it isn't
reporting the correct stack-trace now. This change corrects those
cases. Even if that's a minor BC break, it corrects an error.

By my analysis, that leaves only the 4th case (not throwing) as a
potentially serious BC break. If somebody is presently constructing an
Exception just as a means of extracting a stack-trace, this would no
longer work. But there is a more direct way of doing that, which is
debug_backtrace(), so likely the number of cases of this in the wild
are few, and easy to correct, and likely with backwards-compatibility
of the corrected code in all cases.

I can't think of any other case where this change affects anything,
but please correct me and point out any other case you can think of?

As for Exception::getTrace(), I would propose the signature be changed
to Exception::getTrace($index = 0) which would be backwards compatible
in API terms - the default argument of 0 for $index would return the
most recent stack trace, whereas an index of 1 would return the
previous stack trace, e.g. after a re-throw.

Exception::getTraceAsString() could either change in the same way, or
render out all collected traces (which might be a bit much) or could
simply render the most recently collected trace - I'm not sure which
is best or most compatible.

Exception::getLine() could either return the line-number of the first
or last collected stack-trace - returning the first collected
line-number is more compatible, consistent with "return new" and
re-throwing, but returning the last line-number might be more correct,
I'm not sure.

Would a change like this require an RFC (and a vote) or is it arguably
a bug? (I'm guessing this change is too substantial to be considered a
"bug fix"?)


On Mon, May 23, 2016 at 10:12 AM, Julien Pauli <jpauli@php.net> wrote:
> On Sat, May 21, 2016 at 8:16 PM, Rasmus Schultz <rasmus@mindplay.dk> wrote:
>> Alright, so forget my comparison with other languages.
>>
>> My other points remain:
>>
>> Presently, "throw new" is treated as though it was one statement.
>> That's not the case. We have deferred throws and factory methods for
>> exceptions, and we have re-throws, so collecting the stack-trace at
>> construction time doesn't work.
>>
>> The construction site would only be relevant if "throw new" was in
>> deed a single statement.
>>
>> Recording the actual throw site is clearly the goal - the current
>> implementation is betting on "throw" and "new" happening at the same
>> site, which is merely circumstance.
>>
>> Ideally, an Exception should collect another stack trace for each
>> successive throw, which would enable you to trace not only the
>> original site, but the flow through any exception-handlers that might
>> have re-thrown the same Exception.
>>
>> As is, there is no information collected on throw, and thereby no
>> evidence or record of possible re-throws - on top of the fact that you
>> may be collecting and looking at bogus stack-traces from factory
>> methods or exception mappers.
>>
>>
>> On Fri, May 20, 2016 at 11:06 AM, Rowan Collins <rowan.collins@gmail.com> wrote:
>>> On 20/05/2016 08:22, Niklas Keller wrote:
>>>>
>>>> 2016-05-20 4:13 GMT+02:00 Jesse Schalken <me@jesseschalken.com>:
>>>>>
>>>>> The top frame is the construction (get_error) and the site of the throw
>>>>> (do_throw) doesn't appear in the stack at all.
>>>>>
>>>>
>>>> The comparison with JavaScript isn't a good one, since you can throw
>>>> everything in JS. If they didn't provide the stack trace upon throw, you
>>>> would not have a stack trace at all if you throw a plain string.
>>>>
>>>
>>>
>>> That explanation justifies completely the opposite behaviour to what Jesse
>>> described.
>>>
>>> According to MDN [1] the "stack" property is completley unstandardised, and
>>> some engines may indeed populate it on throw, but there's no hint on that
>>> page that they'll attach it to anything not constructed as an Error.
>>>
>>> So it's not a great comparison for either side (note that it was originally
>>> brought up by Rasmus as an example where it *does* come from the throw site)
>>> because the language doesn't actually guarantee you a stack trace at all.
>>>
>>> [1]
>>> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/stack
>>>
>>> Regards,
>>> --
>>> Rowan Collins
>>> [IMSoP]
>>>
>>> --
>>> PHP Internals - PHP Runtime Development Mailing List
>>> To unsubscribe, visit: http://www.php.net/unsub.php
>>>
>>
>> --
>> PHP Internals - PHP Runtime Development Mailing List
>> To unsubscribe, visit: http://www.php.net/unsub.php
>>
>
> Hi.
>
> I explained that in my article detailing Exceptions from internal ,
> http://jpauli.github.io/2015/04/09/exceptional-php.html
>
> I admit it would be more logical to collect the trace in the
> ZEND_THROW Opcode instead of in the create_handler of the Exception
> class.
>
> That would break backtraces, but we already broke them in PHP 7.0
> So we could think about it for a 8.0 ideally , or a 7.1 ; I'm not
> sure. Anyway, that needs more debate ;-)
>
>
>
> Julien.Pauli