Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#8 | twig-error-useful.png | 261.35 KB | Jacine |
#6 | twig-error-issue.png | 26.64 KB | Jacine |
#2 | 2706683-2.patch | 671 bytes | dawehner |
Comments
Comment #2
dawehnerFatal quick, fatal hard
Comment #3
star-szrComment #4
davidhernandezCan we have an example of the something useful that can't be done inside a template because of this?
Comment #5
dawehnerWell, one example was a mistake in html.html.twig, as this won't be displayed at all, just on the next request.
Comment #6
JacineI 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
php.ini
settings.local.php
Symptoms
Failed Attempts to Recover
Thinking it could be all the usual issues, I tried:
core/rebuild.php
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.
Comment #7
dawehnerI 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.Comment #8
JacineAgreed. 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.
Comment #9
JacineBeen 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
Comment #10
star-szrThis 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().
Comment #11
star-szrComment #12
xjmThanks @Jacine for documenting the impact of this so thoroughly and testing the patch; it really makes it clear why this is a good fix!
Comment #13
dawehnerEven 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
Comment #14
star-szrIt's more of a just in case, in general I agree though. Thanks for the CR @dawehner!
Comment #16
star-szrCommitted ae015a0 and pushed to 8.2.x. Thanks!
Tweaked and published the CR as well.
Comment #17
dawehnerIs there any change that this lands in
8.1.x
given that it improves the life of themers on a day to day base?Comment #19
RainbowArrayI 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.
Comment #20
dawehnerI'm quite sure this could be backported as it is
Comment #21
dawehnerI would have expected someone to ask for a test, but it seemed to be alright ...
Comment #23
xjm@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!
Comment #24
xjm@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?
Comment #25
dawehnerWell, 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.
Comment #26
RainbowArrayThe 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.
Comment #27
star-szrFirst, 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.
Comment #28
star-szrActually it looks like you can do it in page.html.twig too, I did the test incorrectly.
Comment #31
xjmAnd here we are at 8.2.x with 8.1.x EOL. Thanks everyone!