Problem/Motivation
Since symfony/http-foundation 5.1: Retrieving a non-string value from "Symfony\Component\HttpFoundation\InputBag::get()" is deprecated, and will throw a "Symfony\Component\HttpFoundation\Exception\BadRequestException" exception in Symfony 6.0, use "Symfony\Component\HttpFoundation\InputBag::all($key)" instead.
3x in QueryArgsCacheContextTest::testGetContext from Drupal\Tests\Core\Cache\Context
Related changes
- https://github.com/symfony/symfony/pull/34363/files#diff-9d63a61ac1b3720...
- https://github.com/symfony/symfony/pull/34363/files#diff-51a7b6b88afee9f...
Steps to reproduce
Run a test
Proposed resolution
Use approach from SF https://github.com/symfony/symfony/pull/34363/files#diff-0c1327f76881336...
Use request->query->all() instead of request->query->get().
Where the query isn't guaranteed to be set, start using request->query->has().
In MediaLibraryState, re-implement Symfony's deprecation and behaviour, so that any contrib or custom code interacting with the class (unlikely) updates to the new API.
User interface changes
no
API changes
no
Data model changes
no
Release notes snippet
no
Comment | File | Size | Author |
---|---|---|---|
#84 | interdiff.3162016.81-84.txt | 1.1 KB | longwave |
#84 | 3162016-84.patch | 7.89 KB | longwave |
#71 | 3162016-71-test-only.patch | 3.47 KB | longwave |
Issue fork drupal-3162016
Show commands
Start 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:
- 3162016-symfony-inputbag changes, plain diff MR !462
Comments
Comment #2
catchComment #4
longwaveWe don't know until we retrieve it if it's a string or an array here, so I think we have to use this technique instead, as per https://github.com/symfony/symfony/pull/34363/files#diff-0c1327f76881336...
Comment #5
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedThere are some more to check:
Replacing get() with all() method in all files as listed above , kindly review.
Comment #6
longwaveWe only need to replace it in the case where we don't know if the value is a scalar or an array. For example I don't think that 'destination' can be an array here?
Similarly here, 'weight' is a scalar and not an array.
TranslateFormBase is the only one I'm not immediately sure about.
Comment #7
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 for Drupal India Association commented@longwave , i got your point and i have checked in core only TranslateFormBase needs replacement in this case IMO.
Comment #8
andypostComment #9
andypostThere's more places in 9.1.x - 230
get()
on request bags (using phpstorm search)But we should replace only usage in
request
,query
,cookies
which been replaced with newInputBag
See https://github.com/symfony/symfony/pull/34363/files#diff-9d63a61ac1b3720...
Updated IS with proposed resolution
2 tricky places
-
\Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber::generateContext()
I use to add local variable to make condition evaluation the same, becausehas()
could fire for empty array passed in-
\Drupal\media_library\MediaLibraryState
(getAllowedTypeIds()
andgetOpenerParameters()
as class extendsParameterBag
we can skip modify this methods but infromRequest()
method there's$state->replace($query->all());
which could be incompatible withInputBag
from query stringThe interdiff as against #4
No reason to change it, the
$value
used as array key so it must be stringComment #10
andypostextra wrapping parentheses () could be skipped but I've added them for readability
Comment #12
longwaveAs I understand it, here
ajax_page_state
will always be an array, in which case we should call->all('ajax_page_state')
. This is a security hardening, so an exception will be thrown if you call get() for an array or all() for a string - we should only use->all()[...]
when we really accept either string or array.However, all() with an argument also only works from SF 5.1, so should we postpone until then? Otherwise we are bypassing this security improvement and we should add a followup to fix it when we don't need BC with Symfony 4.
Comment #13
longwaveOr could we add a BC layer here where we call
->all('ajax_page_state')
if the object is an InputBag and fall back to->get('ajax_page_state')
for SF4?Comment #14
andypost@longwave Thanks for review, missed the point that default for
get()
isNULL
so changed back that hardening and now broken tests passed.As of
all()
it's since SF 2.x forParamerterBag()
so BC is kept (also works for 8.9 core) now https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/HttpFo...Let's see what bot will tell
Comment #15
longwaveWhat I meant was that in 5.1 InputBag::all() now takes an argument: https://github.com/symfony/http-foundation/blob/5.1/InputBag.php#L48-L51
This is not hardened, ie. if ajax_page_state is a string it will return it, even though we expect an array. As per https://github.com/symfony/symfony/pull/34363 it seems we should only use
->all()[...]
if we really do not know if it is an array or a string.Because of the return type on
all()
this will error if ajax_page_state is a string, but we cannot use this until SF 5.1.Comment #17
Gábor HojtsyCleaner title and fix tag :)
Comment #18
andypostComment #19
catch@longwave #12/#15:
InputBag only exists in Symfony 5.1 at all - meaning on Symfony 4 to 5.1, $request->request is either a ParameterBag (no argument for ::all()) or if we're on Symfony >= 5.1 we have an InputBag which accepts the argument. If that's correct, can we not check instanceOf(InputBag) and pass the argument or not depending on that?
If that's wrong somehow and we really can only start using the new API from 10.x onwards (or if it's a lot more complicated to use it in 9.x), I think we should open a follow-up for it and proceed here so there's less to do later.
Comment #20
longwaveYeah that is what I suggested in #13, it's a bit messy though as there are a lot of calls.
Alternatively can we wrap/alter the object early in the request cycle to our own forward-compatible subclass?
Comment #21
catchOK so three options:
1. Current patch. Forward compatible but not-optimal - will need a follow-up to use the argument.
2. Scatter instanceof around the code base, then remove them again.
3. Subclass either Request or InputBag/ParameterBag or both, and swap this out early for forward compatibility. Update all the calls to use the forward compatible version. Remove the forward-compatibility layer in 10.x
#1 seems preferable to #2. And #3 seems like the best option overall, but IMO we should open a follow-up for that and commit #1 here. The follow-up for #1 would also need to be opened, and #3 will just determine whether it can be committed to 9.x or 10.x
Comment #22
catchI've gone ahead and created both follow-ups: #3198594: Forward compatibility layer for InputBag and #3198596: [PP-1]Update usages of InputBag::all() to take a named parameter.
Comment #23
longwaveSo to test with SF5 this is #14 with #3161889: [META] Symfony 6 compatibility applied but with the two deprecations triggered here explicitly removed. There are two known fails on this patch but if there are no additional fails and #14 passes again then this approach is compatible with both SF4 and SF5.
Comment #25
longwaveThis passed as expected (the two fails are unrelated and a known issue) so this is ready to go and then we can work on the followups.
Comment #26
alexpottI was writing a big review of this and asking why we weren't using
all($param)
and then discovered that that is SF5 only. Then I went and read the earlier discussion from @catch and @longwave. Part of me feels like there is another option that feels worth discussing. This would be to add the deprecation to the skipped deprecations and so we can be SF4 and SF5 compat and then do this issue once SF5 is our minimum compatibility. If we want to do something different then my preference would be to add the forward compatibility layer first and then do this issue so once SF5 is the minimum compatibility we only need to remove the forward compatibility layer.Comment #27
alexpottDiscussed with @catch - we agreed to postpone on #3198594: Forward compatibility layer for InputBag
Comment #28
catch#3198594: Forward compatibility layer for InputBag is making good progress so I think it's OK to postpone this on the change there.
I'll go ahead and close #3198596: [PP-1]Update usages of InputBag::all() to take a named parameter since it'll be redundant if we do things this way.
Comment #29
andypostDependency commited
Comment #30
longwaveThis can now use the new argument to
all()
where appropriate.Comment #32
longwaveOpened a merge request. There are a handful of cases where we handle both a scalar and an array, so we need to use
all()['key']
for those cases.Once this is green we should try a test patch with SF5 as well.
Comment #33
catchTest failures look real, so moving back to needs work.
Comment #34
longwaveFixed many of the failures, let's see what testbot thinks. MediaLibraryState currently extends from ParameterBag, to change it to extend from InputBag (which is needed for the tests to pass) I had to remove the
final
from InputBag for now.Comment #35
andypostneeds to fix cspell and use
Comment #36
longwaveFixed the use statements.
Comment #37
longwaveNW for the last two failures.
Comment #38
longwaveI think this should be green now.
Comment #39
alexpottI think rather than changing the final - we should copy the all() code to MediaLibraryState. If we remove the final then removing the class is harder because someone will extend it like MediaLibraryState.
Comment #40
longwaveAddressed #39, also reworked MediaLibraryState so it can convert ParameterBags by itself the same way the kernel does, which avoids changing the test.
Comment #41
longwaveAddressed @catch's point about ParameterBag, I think this is the best solution as it could be either type of InputBag and we only want to replace it if it is not.
Comment #42
catchThat looks right to me now.
Comment #43
Kristen PolThanks for the updates. @catch says it looks good and tests pass, so I'm not clear if anything else is needed here, but I took a look anyway. Applied patch and:
1) Checked for
getCurrentRequest()->query->get()
and found none left.2) Checked for
request->query->get()
and found a bunch.3) Checked for
getCurrentRequest()->request->get()
and found none left.4) Checked for
request->request->get()
and found a bunch.I'll try to check in with @longwave on this :)
Comment #44
andypostI find changes to empty() from isset() strange, or default values require it now?
Comment #45
longwave@andypost all() always returns an array, isset() on any array (even empty) is true, so we need to use empty() instead.
Comment #46
longwave@andypost we could change many of those
isset/empty()
checks to->query->has()
which would make it cleaner?Comment #47
andypost@longwave great idea! Surely much clearer
Comment #48
longwaveNW for #45-47, will try to get to this this week
Comment #49
longwaveChanged empty() checks to use ->has() instead.
Arguably this could be the job of the router; I don't think .routing.yml lets you specify request/querystring parameters that must exist, but it seems like it could be a cleaner way of doing each of these cases - worth opening a followup?
Comment #51
catchRandom test failure, not sure what happens with retests on merge requests.
I looked through this again and it all looks fine. Was wondering a bit if it was possible to split out the MediaLibraryState changes from the rest of the patch, but not sure that really helps here - we're already in a multiple-patch process to deal with the InputBag changes.
We didn't have a CR for the overall changes, so I've added one, and linked this issue.
Also opened #3213140: Allow route parameters to be specified as required as a follow-up.
I think this is ready, so marking as such.
Comment #52
alexpottThe fail is unfortunately not random.
Comment #53
longwaveThanks for reviews, feedback addressed.
Comment #54
daffie CreditAttribution: daffie commentedThe remarks of @alexpott have been addressed.
I have updated the CR.
For me it is RTBC.
Comment #55
daffie CreditAttribution: daffie commentedAll Symfony 6 issues are critical.
Comment #56
alexpottCommitted and pushed 8b8fc1eff7 to 9.3.x and 8f0f57f3e5 to 9.2.x. Thanks!
Comment #59
chr.fritschThis change is breaking the entity_reference_action module. I have a fix for the module #3216944: Saving modals doesn't work anymore, but maybe it should be fixed here.
Comment #60
alexpott#3216944: Saving modals doesn't work anymore shows that we have broken sub-requests... I'm going to revert this so 9.2.x is not affected and perhaps we can work out a solution that works for sub-requests to.
Comment #63
huzookaRe #60 Isn't this a great opportunity to test them as well? Or it is out of scope?
Comment #64
alexpott@huzooka we definitely need to add test coverage of what broke the contrib module.
Comment #65
longwaveI think the underlying issue here is #3198594: Forward compatibility layer for InputBag - there we swap InputBag in place of ParameterBag on the main request only, then this issue depends on that one, but the swap doesn't cover the case of subrequests.
Comment #66
longwaveSo maybe we can do something like this, as long as others use
Request::create()
and notnew Request()
I think this will work.Comment #67
longwaveOh, except we already override the request factory in
DrupalKernel::setupTrustedHosts()
.Comment #69
catchCrediting Chi from two much older, but related issues (Drupal not deciding whether it expects an array, string, or both).
Comment #70
Berdir#67 means this is needs work if I understand this correctly. I'd assume that we would need to duplicate that logic within there too?
Comment #71
longwaveAddressed #67. The TrustedHostsRequestFactory logic is only triggered if trusted hosts are set up, so I extended the existing functional test for this case.
Comment #73
daffie CreditAttribution: daffie commentedThe patch looks good. Just one remark:
Why is this added to the test. I think we can remove it.
Comment #74
longwaveSee #71. TrustedHostsRequestFactory is only initialised at all if a host pattern is set, but by default it is empty. See
DrupalKernel::initializeSettings()
:So we need to explicitly set a
trusted_host_patterns
key in the test setup for this code path to be exercised. We could prove this in the test by throwing an exception from the test controller if trusted host patterns are empty, I guess?Comment #75
daffie CreditAttribution: daffie commented@longwave: When I run the test on my local machine the test passes. It also passes when I remove the part from #73. That is why I was asking if it could be removed. If you want to commit this with the part, then I will change the status to RTBC.
Comment #76
longwaveThe test passes, but it is no longer testing one of the code paths where a fix is required in this issue.
If you also remove the change from TrustedHostsRequestFactory then the test will still pass. But then if you put the part from #73 back, and keep the change out of TrustedHostsRequestFactory, then the test should fail.
Comment #77
daffie CreditAttribution: daffie commented@longwave: Thank you for the explanation.
The change for using the The Drupal forward compatibility class for Symfony 5 (Drupal\Core\Http\InputBag) has been moved from the class Drupal\Core\DrupalKernel to file bootstrap.inc. In this way sub-requests also use the forward compatibility class.
There is testing added.
For me it is RTBC.
Comment #79
longwaveNote to committers: the merge request was the reverted version of this code - the patch in #71 is the updated version with the subrequest fix.
Comment #80
longwaveThere is a subtle bug in #71 actually:
This class needs a use statement.
Comment #81
longwaveComment #82
daffie CreditAttribution: daffie commentedUse-statement added.
Back to RTBC.
Comment #83
catchDoes this need a @todo to remove once we require Symfony >=5 or are we leaving it as a permanent test. It looks temporary but I could be missing something.
Same question I think.
Comment #84
longwaveAdded @todos.
Comment #85
andypostI bet both todos need links to get rid of them
Comment #87
catchIn this case I think it's OK - we'll probably grep the codebase for 'Symfony 4' when we drop support.
Committed 022284b and pushed to 9.3.x. Thanks!