Since symfony/http-foundation 7.4: Request::get() is deprecated, use properties ->attributes, ->query or ->request directly instead. See Symfony issue, [HttpFoundation] Deprecate Request::get() in favor of using properties ->attributes, query or request directly
Drupal often uses Request::get() when we accept data via GET or POST; in Symfony 8 this will no longer be allowed and we will explicitly have to check the correct input bag(s).
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !13773
Comments
Comment #2
scott_euser commentedSpotted this as well in the AI module, applying a workaround there for now.
I would imagine there are times like in Views where we may want GET or POST - do we need to wrap Symfony Request in a Drupal Request to continue to provide the more generic get()? Or should we be finding all cases in core and contrib and sorting them individually?
Comment #3
longwaveI suspect we should be being more precise where possible, especially if we know the request was a GET and the data will always be in the querystring for example; there are possible security issues if someone can smuggle in data via an unexpected method. Symfony will likely have done it for good reason, we should dig up the Symfony issue/PR where this was changed and find their reasoning for doing it which might help us.
Comment #4
scott_euser commentedThanks! Here is the PR, it's not too informative unfortunately:
Comment #6
quietone commentedAdd Symfony link to the issue summary
Comment #7
godotislateCommented on the MR, but I remembered in #3399951: ajax_page_state leaks through request in Views Ajax, that the
ajax_page_stateis temporarily copied to$request->attributesinViewAjaxController::ajaxView, so that it doesn't leak through to the GET parameters. So the theme negotiator needs to check attributes as well.Comment #8
godotislateGot tests passing. Used a sledgehammer to simulate
Request::get()when dealing withajax_page_state, and the weirdness in howInputBag::get()andParameterBag::all()work.Comment #9
longwaveThanks for finishing this off. I think we should try to deal with ajax_page_state better, perhaps by explicitly adding it as a request attribute in a single place - we could do more validation, convert it to a value object, etc if we do that as well. But that's probably for a followup?
Comment #10
godotislateYes, I agree that we should deal with ajax_page_state better, maybe in a request subscriber?
Also agreed that it makes sense for a follow up, which I just created: #3560521: Consolidate ajax_page_state from request in a single place early during request handling. Issue title probably could be better, but it works for a stub.
Comment #11
smustgrave commentedGood way to test this one?
Comment #12
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #13
godotislateRebased.
I don't think there's anything to test per se, since this is a small refactor in place, so automated testing should pick up regressions.
That said, you can check:
get()methodComment #14
longwaveIt's also worth checking that the logic still looks correct/consistent where we check query vs request (and in some cases, both). Hopefully the tests cover most cases, but probably not all of them.
Comment #15
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #16
godotislateComment #17
godotislateRebased against main and regenerated phpstan baseline after merge conflict.
Comment #18
longwaveGiven the tests are green I think we should get this in sooner rather than later to try to catch any edge cases we might have missed where things might be in a different parameter bag.
Comment #19
larowlanLeft some observations on the MR - thanks
Comment #20
godotislateReplied to MR comment about ajax page state with a follow up question.
Addressed the other MR comment and rebased.
Comment #21
longwaveThe AjaxPageState middleware already did some modification of the request, so let's just extend that to also put it in a request attribute, then we can always collect it from there. We probably need a change record. Also we should probably deprecate retrieving it from the request/querystring, but not sure how to do that.
I also changed the package manager test to only ever use the query string, as far as I can see that's the only way it was called anyway. #3559628: Allow #[MapQueryParameter] attribute for query string handling in controllers would make this even neater.
Comment #22
longwaveClosed #3560521: Consolidate ajax_page_state from request in a single place early during request handling as duplicate and added a CR for the
ajax_page_staterequest attribute.Comment #23
godotislateA workaround was added to ViewAjaxController in #3399951: ajax_page_state leaks through request in Views Ajax to stuff the AJAX page state into the request attributes so that theme negotiation would still work, but would prevent URLs from being built with ajax_page_state query params. Since this is happening in middleware now, I think it should all be removed from ViewAjaxController.
Comment #24
longwaveAh yeah that makes sense, I wonder if moving all this to attributes (and explicitly removing it from the query string) is a viable solution to #2504709: Prevent _wrapper_format and ajax_form parameters from bleeding through to generated URLs
Comment #25
godotislateI had a thought that since AJAX page state (and other metadata) are being used in AJAX requests, then instead of using query or POST parameters, it's possible to put all that in custom request headers. Even if not done in the current jQuery-AJAX-based architecture, it could be something to look at with HTMX. That said, there'd still probably need to be a query parameter on AJAX GET requests to bust through CDN caches, though maybe it could be a single hashed string instead of all the array parameters. I guess there's also the risk that hosting services filter out custom request headers.
Anyway, MR looks good to me.
Comment #26
longwaveYeah I've been wary of custom headers since #2580191: Firewalls may remove the Ajax verification token header but now HTMX uses them and things have moved on, maybe they're okay?
Comment #28
larowlanCommitted to main - thanks
Comment #32
larowlanBackported to 11.x and published CR