Catch error with entrypointlookups in sub-requests #21
Conversation
Twig_Error_Runtime leads to sub-requests. In EntrypointLookup, the list of previously returned entries is not reset on these requests. Fixes symfony/demo#910
This replaces the fix with an ExceptionListener. When returning full html in a sub-request, already included assets should not be included again.
|
I'm wondering if that's the right way to handle that... if a sub-request is created but then popped off the stack you'd go back to the original one with the wrong returned entries, right? Wouldn't it be better to keep track of the returned entries per request instead? |
Yes, I think so.
Maybe the more specific fix from 6b6b731 is better, as it only covers the use case with |
This reverts commit 0a5e7f9.
|
Yea, this is a complex issue - thanks for checking into it @ckrack.
Yes, I also think this is the best way to do this. I was talking with @ckrack about this, and I thought for a moment that this was NOT the best way - but now I think it is. Basically. the EntrypointLookup should keep track of which assets have been used on a request-by-request basis. We can probably do this by injecting the RequestStack and using the There is still another edge-case, but I think it can just be handled by the user (and some documentation): if you render 2 full HTML pages within one sub-request (e.g. render HTML for a PDF, then render the normal template - odd case, but possible), you will need to manually reset the stack. @ckrack are you able to make the above changes? Thanks |
|
Ping @ckrack! I just merged another PR, which would have probably conflicted with the work that needs to be done here. So, things are clear now. Do you have some time to work on this? Thanks for the attention! |
|
Ok, on third thought, I think this is the right approach - we cannot predict what behavior is correct for sub-requests in general. |
| $exception = $event->getException(); | ||
|
|
||
| // Reset the entrypointLookup list of previously returned entries, as Twig_Error_Runtime will initialise a sub-request | ||
| if ($exception instanceof Twig_Error_Runtime) { |
There was a problem hiding this comment.
Why this error specifically? All exceptions will result in a full page being rendered (by default). So, I think what you’re doing is trying to detect if the error came from Twig, which is the most likely cause of a situation where Encore assets have been rendered already... and THEN an exception happens. Am I right?
If so... I’m not sure it’s a good idea. Regardless of the error, if we try to render a template for an error page, that should probably use a fresh set of Encore assets.
There was a problem hiding this comment.
I’m also trying to think if we should limit this to the master request... does the default exception behavior (rendering an error template) happen only on the master request? That seems likely - I just haven’t checked the code yet.
There was a problem hiding this comment.
I've thought more about this. I think we should:
-
Always reset on exception - not just if there was a Twig error. There is going to be some edge cases (people successfully rendering in Twig, then a listener throws an exception after).
-
We should reset on all requests, not just the master request.
There may still be edge cases - but let's do that and see what happens. Fortunately, this is in "exception" land - so it's already not a normal request.
|
Also, there is now one more complication: #17 added support for multiple builds. So there is now an |
|
Ping @ckrack! Are you still able to work on this? Thanks |
|
Hi @weaverryan ! What can be done to move this forward? Can we help? |
…(tbmatuka) This PR was squashed before being merged into the master branch (closes #74). Discussion ---------- Reset default EntrypointLookup on exception to fix #21 and #73 Since @ckrack seems to be unavailable to continue working on #21, I figured this would be faster :) I don't like having `_default` hardcoded in the listener, but I see no other options right now. I thought about adding a `resetAll()` method to EntrypointLookupCollection, but there were a couple of issues with that: 1. It would either be a BC break if I added it to the interface, or an important method that isn't interfaced and I don't really like either of those. 2. I couldn't even implement it because the container that we get in the collection only has `has()` and `get()` methods, so I couldn't go through it. This would also have to be replaced (and break BC) to implement `resetAll()`. Fixes symfony/demo#910 Commits ------- da36629 Reset default EntrypointLookup on exception to fix #21 and #73
In EntrypointLookup, the list of previously returned entries is not reset
on sub-requests.
This leads to errors when sub-requests return full html pages.
Fixes symfony/demo#910