Fix: Original Throwable get lost during component render

[cuchac]

Q	A
Bug fix?	yes
New feature?	no
Issues
License	MIT

When non-Exception Throwable is thrown during component rendering, original Throwable is lost together with it's stacktrace. Without stacktrace it is hard to debug the problem. This simple change fixes the problem in the same way as in https://github.com/cuchac/ux/blob/f69b9656f318464253590a6575da65f34262d931/src/TwigComponent/src/Twig/ComponentExtension.php#L127 where previous: $e is used instead of previous: $e->getPrevious()

[smnandre]

Hi @cuchac

I remember i almost made the same change .... and reverted because other scenarios made it not better... but don't remember precisely why/when.

So... let's solve this :)

Do you have a simple scenario / use case that we can reproduce to play around ?
I'll check on my side to find why i went back :)

[cuchac]

Hello @smnandre

when I for example access undefined variable inside component PHP file, I will get following error page:

There are two stack traces but the original stack trace that can be used to solve the issue is lost. Without stack trace I can only guess where the problem inside the component is.

[smnandre]

Just wondering, what if you remove entirely this block (i'm not remembering why it's there is the first place)

if (!($e instanceof \Exception)) {
    // ... 
}

[smnandre]

Found the context / reasons here:

• Avoiding double-wrapping of Twig RuntimeError #1132
• [Site] More Functional Code, 2x Complex Live Components Examples, few fixes #804 (review)

[cuchac]

I believe this whole part is useless. Why converting Throwable to Exception? RuntimeError can have Throwable as previous parameter. Wrapping Throwables in Twig RuntimeError is correct in my opinion, but wrapping Throwables in another Exception is useless, it just adds another stacktrace and also causes the problem with lost original stack trace because of $e->getPrevious() error.

[smnandre]

I'm feeling the same, with no absolute certitude of the scenarios that probably justified this in the first place.

[cuchac]

Just wondering, what if you remove entirely this block (i'm not remembering why it's there is the first place)

if (!($e instanceof \Exception)) {
    // ... 
}

If I delete that if, it will fix my issue. I believe it can be deleted.

[cuchac]

Hello @smnandre, I've changed the PR to remove the problematic if.

[smnandre]

@kbond if you have time to check, i'm not sure if the initial reasons still apply.. but it feels like a good idea :)

[cuchac]

@smnandre I've looked at the original commit and I don't see any reason for that if statement. I understand why Twig RuntimeException should be thrown, but I don't understand why "repack" non-exceptions. I believe it was mistake from the beginning.

[kbond]

Thanks for this Joe. I'm not sure why this was done previously but this is the right move.