Problem/Motivation
Follow-up to #2230121: Remove exit() from FormBuilder and #2503429: [PP-*] Allow both AJAX and non-AJAX forms to POST to dedicated URLs.
Drupal currently supports throwing an exception to exit the code flow and force an immediate redirect. This was implemented largely as a stop gap to allow the removal of direct response rendering and exits from core API's allowing better leverage of Symfony's HTTP stack.
Those API's should be refactored to allow common exit flows or redirecting without throwing an exception and then the Exception and its handling can hopefully be deprecated and retired.
Remaining tasks
User interface changes
N/A
Comments
Comment #1
andypostWhy this issue postponed til 9.x?
maybe there's a way to make it in 8.x by providing some BC
Comment #2
catchYes this would be a major performance improvement, so I'd consider it at any point during 8.x if we can do it without causing massive disruption to contrib modules.
Comment #3
Wim LeersThis is also why render caching is disabled for POST requests. Which means that on every single POST request, we re-render a lot of things that with this issue in, could simply be retrieved from the render cache.
Comment #4
neclimdulMy goodness
guysy'all, we use an exception for code flow in formapi?! This was a mess to decipher!So, its turns out the whole rigamarole currently in FormAPI where we throw an exception, catch that exception, then rewrite that exception breaks when you try to use the HttpKernelInterface::handle() flag to turn off exception catching and view a normal PHP/xdebug backtrace. I don't know if that makes this a bug or some related issue but using an exception like this for normal code flow is very broken.
Comment #5
catchMarking duplicate of #2503429: [PP-*] Allow both AJAX and non-AJAX forms to POST to dedicated URLs. This is the original issue, but that one has a more up-to-date plan on it.
Comment #6
catchOnce #2503429: [PP-*] Allow both AJAX and non-AJAX forms to POST to dedicated URLs is fixed, and all core forms are submitted, we'll be able to deprecate support for EnforcedResponseException for 9.x. This is a case where a contrib module update to use the new API could take seconds off a form submission so worth prodding.
Comment #8
fubhy CreditAttribution: fubhy commentedI need to override this behavior for caching specific POST requests (GraphQL queries). Right now this is a PITA because I have to override the entire method bodies for get()/set(). Can we move this hotfix into a helper method on the RenderCache / PlaceholderingRenderCache classes at least?
Comment #9
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#8: Sure, could you open an issue for that?
Comment #15
ndobromirov CreditAttribution: ndobromirov at FFW commentedSorry to add the tag but I got here after getting a bug caused by this exception used for code flow.
I was not expecting that Form-API will throw an exception to handle the submit redirect.
Comment #20
larowlanWe could also add a redirect middleware ala #3214949: Headers have already been sent after upgrade to Drupal 9.2 (can't login) and make that a supported API
The code there could easily be lifted to core
Comment #21
bradjones1Related in other contrib: Commerce uses (abuses?) this for redirects:
https://git.drupalcode.org/search?utf8=%E2%9C%93&search=enforcedresponse...
Comment #22
neclimdul@larowlan definitely seems like a possibility. I think I've done something similar in an event listenert where I needed to modify the response headers not replace the response. You have to play carefully with page cache but I believe its maybe possible here too without adding another middleware?
Reaching back into the past, I have to wonder if part of the reason for the implementation was more about "exiting" instead of the actual redirecting. Like if you're in a build or validation form step and need to redirect _before_ any additional code can run. I think that's a failing of API design. No offense intended, we can only solve so many problems at once and we didn't fully know what a lot of D8 was going to look like. I certainly didn't offer a better option.
At this point seems like that's probably solvable by paying down some technical dept on those API's. I do wonder if we've created one of those "this feature exists but if you're using it you're doing it wrong" sort of features we'll have to continue support for contrib for a while though.
@bradjones1 Good find. I'm not currently super familiar with Commerce's code but skimming, it seems to into this exact problem in its own API design. Their main use seems to be in its wizard builder system where plugins are responsible for a lot of validation of "should the user be here" and then triggering a "no they should be on another step" redirects. The design requires this to happen primarily in the _build_ step, and that can only return form array. They use the exception as a path out that provides a "secondary" return type but also a way of exiting the code flow.
Its clever and convenient but probably not the best approach because of the exception but also it mixes a lot of concerns in the build method. This also seems solveable but probably requires a lot of planning and API design. If I can naively suggest a solution, this seems like a common pattern on their plugins, so a method for prevalidation(and a new interface?) that can return a step id, or a redirect response, or maybe a magic step id redirect response. This would also probably allow for the consolidation of a lot of patterns and boiler plate into commerce itself instead of in the plugin build methods.
Comment #24
gabesullice*nods* knowingly to the next poor soul to land on this issue after an hour or two of some crazy step debugging.
We share a special bond now.
Comment #28
catch#3395776: Make POST requests render cacheable is making POST requests render cacheable without changing form discovery, which doesn't affect this issue except it means that #2503429: [PP-*] Allow both AJAX and non-AJAX forms to POST to dedicated URLs and this issue are more clean-up than performance improvements now.