Problem/Motivation

Currently we convert theme exceptions to drupal messages, which for example makes it impossible to do something useful inside the html templates.

Proposed resolution

Don't convert them.

Remaining tasks

User interface changes

API changes

Data model changes

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

dawehner created an issue. See original summary.

dawehner’s picture

Status: Active » Needs review
FileSize
671 bytes

Fatal quick, fatal hard

Cottser’s picture

Issue tags: +Twig
davidhernandez’s picture

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

dawehner’s picture

Well, one example was a mistake in html.html.twig, as this won't be displayed at all, just on the next request.

Jacine’s picture

FileSize
26.64 KB

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):

Screenshot of Drupal error message, provided way too late

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.

dawehner’s picture

I think the main reason why you have't seen anything is that you changed html.html.twig, in general though I think we should fail, when there is a reason to fail for.

Jacine’s picture

FileSize
261.35 KB

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.

Useful Twig error

Jacine’s picture

Status: Needs review » Reviewed & tested by the community

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

Cottser’s picture

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().

Cottser’s picture

Status: Reviewed & tested by the community » Needs work
xjm’s picture

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!

dawehner’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record

Even I don't really see a change here, noone would ever have to adapt code or configuration due to that, meh: https://www.drupal.org/node/2713329

Cottser’s picture

Assigned: Unassigned » Cottser

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

  • Cottser committed ae015a0 on 8.2.x
    Issue #2706683 by dawehner, Jacine: Don't convert twig exceptions to...
Cottser’s picture

Assigned: Cottser » Unassigned
Status: Reviewed & tested by the community » Fixed

Committed ae015a0 and pushed to 8.2.x. Thanks!

Tweaked and published the CR as well.

dawehner’s picture

Is there any change that this lands in 8.1.x given that it improves the life of themers on a day to day base?

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

mdrummond’s picture

Version: 8.2.x-dev » 8.1.x-dev
Status: Closed (fixed) » Patch (to be ported)

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.

dawehner’s picture

Status: Patch (to be ported) » Reviewed & tested by the community

I'm quite sure this could be backported as it is

dawehner’s picture

I would have expected someone to ask for a test, but it seemed to be alright ...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2706683-2.patch, failed testing.

xjm’s picture

@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!

xjm’s picture

@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?

dawehner’s picture

@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?

Well, from a pure functionality perspective I think we need no test. Its standard PHP behaviour. One could make a point of a regression test, but yeah, I hope we don't come up again with such a stupid idea. The drupal message is potentially shown on actual production sites, but in case its shown, I would almost argue the site is broken already:

a) A message is shown for users, which should not see it
b) A template is not fully rendered, which could be even more confusing, especially depending on which level this was.

On the other hand a WSOD is pretty brutal in terms behaviour, so I would lean towards not committing it to 8.1.x. 8.2.x is not that far away.

mdrummond’s picture

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.

Cottser’s picture

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.

Cottser’s picture

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

  • Cottser committed ae015a0 on 8.3.x
    Issue #2706683 by dawehner, Jacine: Don't convert twig exceptions to...

  • Cottser committed ae015a0 on 8.3.x
    Issue #2706683 by dawehner, Jacine: Don't convert twig exceptions to...
xjm’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs work » Fixed

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.