Render the page after a non-AJAX postback - #1518
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ec15e7c to
f7896ba
Compare
A non-AJAX postback whose handler returns nothing (normalized to true) never reached the page action and errored on an undefined variable. The dispatch now falls through to the page action, guarded by a flag set where actions actually execute, and excluding a handler that names the action itself, which handler dispatch already invoked directly. When the action already ran during handler dispatch, its result was not retained and running it again would repeat its side effects, so those paths now fail with a meaningful message where they previously failed with the undefined variable error. The fixture tests count executions across the render, widget, discovery and self-handler paths; each guard was validated against a variant without it.
f7896ba to
a33719c
Compare
|
@austinderrick can you explain the why of why you need this? |
|
Hey @LukeTowers : In another draft branch, I'm playing with PHPStan level 1 findings. This popped up there and I noticed it was more "bug like" versus syntax, so I pulled it out. It first surfaced there as The variable is undefined because the postback branch never reaches the page action, so a bare This PR fixes the dispatch instead. Plain handlers fall through and render the page like the CMS controller does, and the paths that can't render (widget/discovery, where the action already ran view-suppressed) throw a real error in place of the more cryptic one. Happy to drop this and do the one-liner in the PHPStan branch if you'd rather not touch the dispatch here. |
|
@austinderrick I still don't understand the actual real world problem that is being solved here, your description reads to me like static analysis / AI doesn't like how something is done right now and thinks it's problematic; but I still don't understand what the actual real world problem is that is trying to be fixed by this. Could you provide an example of a scenario that doesn't work right now that this change is intended to fix? Touching stuff related to the internal routing & especially the AJAX framework routing is extremely risky; I've had to deal with a lot of security reports related to routing over the past several months. |
2 participants
Submitting a backend form without AJAX runs the handler and then returns a blank page. The handler result normalizes to
true, so the dispatch chain skips the page action entirely.The CMS controller already falls through to its page render in this case, and the backend controller now does the same. The new regression test fails without the fix, and a second test covers handlers that suppress the view.