Updated: Comment 0
Problem/Motivation
As written in #1998638-42: Replace almost all remaining superglobals ($_GET, $_POST, etc.) with Symfony Request object we figured out that the ExceptionListener from symfony does not really allow to use POST requests on exception pages.
This for example blocks us from using login forms on 403 pages or search forms on 404 pages, which is a quite common usecase.
As asked on https://github.com/symfony/symfony/issues/9660 we should better extend the existing one instead of "fixing" the one from symfony.
Proposed resolution
Implement our own exception listener / controller that allows to use post requests as well. Therefore the subrequests need to pass along the request type as well as request information if existent.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#54 | drupal_2147153_53.patch | 11.57 KB | Xano |
#32 | interdiff.txt | 704 bytes | dawehner |
#27 | exception-2147153.patch | 9.41 KB | dawehner |
#27 | interdiff.txt | 2.05 KB | dawehner |
#20 | exception-2147153.patch | 9.41 KB | dawehner |
Comments
Comment #1
dawehnerThis works pretty fine for me.
Comment #3
dawehner1: exception-2147153.patch queued for re-testing.
Comment #4
larowlanLooks good, great work.
Those unit tests are slick.
Comment #6
chx CreditAttribution: chx commentedThis is odd. I guess I can get behind $_GET becoming
query
, after all the standard calls it query (RFC 1738 too), it's just PHP that names it $_GET. But$_POST
becoming request?? That's a bug.$_REQUEST
exists and a whole different animal. If this is a Symfony thing, we need a critical bug filed over this, if this is a typo in the patch then the patch need to be fixed.Comment #7
chx CreditAttribution: chx commentedSigh. Really. #2147401: Symfony is braindead: $_POST became request filed.
Comment #8
chx CreditAttribution: chx commentedI can't even put words my dismal that such a hopeful issue like this leads to another issue where I am fighting Symfony. I don't want to fight :(
Comment #9
dawehner1: exception-2147153.patch queued for re-testing.
Comment #11
dawehnerInteresting those failures work fine on my system.
Comment #12
dawehner1: exception-2147153.patch queued for re-testing.
Comment #13
damiankloip CreditAttribution: damiankloip commentedShall we just go all out and use === here?
Might as well make them match and use 'GET'. I know symfony does an strtoupper() on the method anyway but...
This isn't re throwing anything
Can we get some @group action on here?
Comment #14
Crell CreditAttribution: Crell commentedNitpick: GET, not get, for consistency. (I don't think it matters to the code, but we should be consistent.
This clearly still has the original Symfony code in it; it's not clear what part you're changing, though. Can you document that, so we know what to review more effectively?
Comment #15
dawehnerThere we go.
Comment #16
Crell CreditAttribution: Crell commentedThere we go.
Comment #17
catch15: wscci-2147153.patch queued for re-testing.
Comment #20
dawehnerRerolled.
Comment #22
dawehner20: exception-2147153.patch queued for re-testing.
Comment #24
dawehner20: exception-2147153.patch queued for re-testing.
Comment #25
Crell CreditAttribution: Crell commentedStill just HEAD chasing. Still RTBC.
Comment #26
catchNone of the inline comments here match coding standards.
Comment #27
dawehnerFixed them. Yeah I should have not just c&pasted the original code.
Comment #30
dawehner27: exception-2147153.patch queued for re-testing.
Comment #32
dawehnerThis just fixes a single of these failures. Batch is still broken with this.
Comment #34
dawehnerFixed many failures.
Comment #36
dawehner#2176669: Stop persisting the url_generator fixed all the remaining failures.
Comment #38
dawehner36: exception-2147153.patch queued for re-testing.
Comment #39
damiankloip CreditAttribution: damiankloip commentedThis doesn't re throw.
What's the reason for these changes? (mainly out of interest).
oops
Do you think we should also add an assertion to test that the system path destination gets set here?
I guess this will no longer apply anyway, as #2176669: Stop persisting the url_generator is in, and this has that rolled into it.
Comment #40
Crell CreditAttribution: Crell commented36: exception-2147153.patch queued for re-testing.
Comment #42
dawehnerThank you for your review!
Note: This is coming 1by1 from symfony, so I think we should not deal with it here, but maybe add an issue to upstream.
Previously it set example.com to the request object which was used in the test on 403/404 pages.
This would just makes sense to be used on a potential unit test of the ExceptionController.
Comment #43
dawehnerForgot to upload the file.
Comment #45
dawehner43: exception-2147153.patch queued for re-testing.
Comment #46
dawehnerAdapted the title.
Comment #47
Crell CreditAttribution: Crell commentedLet's file an upstream issue. Otherwise, I think this can move forward again.
Comment #48
dawehner@crell
Have a look at https://github.com/symfony/symfony/issues/9660
Comment #49
Crell CreditAttribution: Crell commentedI was referring to this:
Comment #50
alexpottIn #2161397: Update to Symfony 2.4.1 we updated Symfony which added a new protected method duplicateRequest(). I think this means we can just override the new duplicateRequest() function rather than the onKernelException() function.
Comment #51
alexpottLike so.
Comment #52
dawehnerHAHAHA, busted: https://github.com/symfony/symfony/commit/6af28015d4d102145bd1a4541cdf7f...
Just fixed the 3 codestyle issues.
Comment #53
alexpottexception-2147153_6.patch no longer applies.
Comment #54
XanoRe-roll for the
synchronized
property incore.services.yml
.Comment #55
catchCommitted/pushed to 8.x, thanks!
Comment #56
jhodgdonI think that this commit broke the patch for #1366020-59: Overhaul SearchQuery; make search redirects use GET query params for keywords -- The explanation for what is happening is on comment #59, having to do with attempting to use $form_state['redirect_route'] from a form in a block on a 404 page.
I'm not absolutely positive yet that this commit is the cause of that patch failing now, but the form block redirect code in that patch was working on Jan 25th and it is now not working, so damiankliop and timplunkett suggested that this commit is probably what broke it.
Comment #57
jhodgdonThe problem noted in #56 has been filed as a new issue:
#2206909: Regression: Form submit redirects from 403/404 pages are no longer possible