Don't convert twig exceptions to drupal messages

Can we have an example of the something useful that can't be done inside a template because of this?

I think this is needed. I ran into this the other day, and lost 5 hours of trying to figure out what was going on. Twig was the last place I would have, and did look for a problem.

Environment

1. PHP 7 / MAMP.
2. Errors configured to display in php.ini
3. Error reporting configured in Drupal site via settings.local.php
4. xDebug enabled
5. Twig Debugging enabled

Symptoms

1. WSOD
2. No errors present on screen
3. No errors present showed PHP, Apache or MySQL logs

Failed Attempts to Recover

Thinking it could be all the usual issues, I tried:

1. Multiple cache rebuilds
2. Manually truncated all cache* tables
3. Ran core/rebuild.php
4. Restarted AMP
5. Updated Drush
6. Hacked index.php with trick from Alex Pott

None of this had any affect.

It wasn't until I inspected previous commits line by line, that I found the actual problem was this line in a Twig template:

{% include '/themes/THEME/image/icons.svg' %}

This failed (very silently) because I had relocated this file, and forgot to update the path in the template it was referenced from. It wasn't until AFTER I fixed the issue, that I got any feedback that there was a problem. which obviously at that point is useless (and frustrating):

I'm not sure why these errors would ever be suppressed in the first place. If there is a good reason for not letting this throw a fatal error, I would hope that at the very least, the error could be allowed when Twig debugging is enabled. A debug mode, without actual debugging ability is not very useful.

Agreed. With this patch applied, I'm immediately alerted to the problem. It would have taken 30 seconds to resolve the issue as opposed to hours.

Been using this for the past few days. It has helped me a few times already, and I would like others to have the same help ASAP. :D

This could use a small change record, otherwise looks good. FWIW this was added in #2226207-65: Make 'template' the default output option for hook_theme().

Thanks @Jacine for documenting the impact of this so thoroughly and testing the patch; it really makes it clear why this is a good fix!

It's more of a just in case, in general I agree though. Thanks for the CR @dawehner!

Committed ae015a0 and pushed to 8.2.x. Thanks!

Tweaked and published the CR as well.

I just wasted a couple hours this afternoon because of this error. Same thing, problem with a filename, white screen of death, almost no way to track down the bug.

This is a three line patch. Can we please backport this to 8.1? I can't see how this would be disruptive, but not backporting this could result in hours and hours of developer time for people who run into this between now and when 8.2 is released.

@mdrummond, changes to the exception handling flow can also be disruptive, because calling code may catch exceptions and respond to that, so if we change the exception (or start catching it) then that causes problems for their code flow. Or if an exception is currently caught (as in 8.1.x without this patch) and then we stop catching it (as in 8.2.x after this patch), that can obviously be very disruptive when suddenly there are 500 errors on production sites for a patch-level update.

In the case of this issue, as @Jacine outlined, we have a situation where the attempt to "gracefully" handle the exception is just completely failing anyway. The question is, If we backport this to 8.1.x, is there any minute chance that an exception not previously thrown on a production site might now be thrown? Any teeny-tiny chance under any circumstances? If the answer is "yes" we should not backport it. However, given how painful this is for people, it is worth asking. Thanks!

@dawehner, I was wondering about that too, but I was feeling that it would be like testing Twig's exceptions. Do you think we should test it?

The current behavior is that if there is a Twig exception, there's a white screen of death. Yes, if you switch to another theme, you can sometimes see the Drupal message, but for the most part, if you have a theme generating a Twig exception, your site will be non-functional.

So I have a hard time seeing how there would be a production site with a Twig template generating a fatal exception.

The incidents where I've seen these errors have been in html.html.twig, which is when I've most often had a need to do an include statement: errors in those file paths seem to be what is most likely to trigger an exception. I don't know if the exception happened in another template if you would seem the same WSOD, but I suspect you would. Viewing source on the WSOD page showed zero HTML: the HTML that happened before the error wasn't there. So I'd guess a WSOD would be likely in other templates too.

I understand not wanting to disrupt production sites. I'd think this change would be much more likely to help than to hurt, but I can't say that I know that 100%. Cottser, lauriii or joelpittet might have more insight on that.

The current behavior is that if there is a Twig exception, there's a white screen of death.

First, based on the comments on this issue so far it seems to only be an issue in html.html.twig. Second, I tested and it's very possible to get a non-WSOD error from other templates, even page.html.twig. If I'm not mistaken a production site could easily have one of these errors and have error reporting turned off. I think html.html.twig is the only one that cannot have a "graceful" exception in 8.1.x (and 8.0.x).

@mdrummond I'm sorry that you (and @Jacine and probably others not on this issue) had to spend time debugging an issue that would have been easier to fix in 8.2.x. I'm still hesitant to commit this to 8.1.x for the reasons @xjm stated. Even though proper error reporting for all templates is a win I don't want to break someone's 8.1.x site, especially because we have no evidence that this affects templates other than html.html.twig. I understand this is where you want to use includes but there are many other ways to produce Twig exceptions than using includes.

Actually it looks like you can do it in page.html.twig too, I did the test incorrectly.

And here we are at 8.2.x with 8.1.x EOL. Thanks everyone!