[Symfony 6] Retrieving a non-string value from "Symfony\Component\HttpFoundation\InputBag::get()" is deprecated

We 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...

There are some more to check:

core/lib/Drupal/Core/Entity/EntityForm.php
core/modules/block/src/BlockForm.php
core/modules/field_ui/src/Form/FieldConfigEditForm.php
core/modules/locale/src/Form/TranslateFormBase.php
core/modules/settings_tray/src/Block/BlockEntitySettingTrayForm.php

Replacing get() with all() method in all files as listed above , kindly review.

1. +++ b/core/lib/Drupal/Core/Entity/EntityForm.php
   @@ -238,7 +238,7 @@ protected function actions(array $form, FormStateInterface $form_state) {
   -        $query['destination'] = $this->getRequest()->query->get('destination');
   +        $query['destination'] = $this->getRequest()->query->all()['destination'];
   

   We 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?

2. +++ b/core/modules/block/src/BlockForm.php
   @@ -176,7 +176,7 @@ public function form(array $form, FormStateInterface $form_state) {
   -    $weight = $entity->isNew() ? $this->getRequest()->query->get('weight', 0) : $entity->getWeight();
   +    $weight = $entity->isNew() ? $this->getRequest()->query->all()['weight'] ?? 0 : $entity->getWeight();
   

   Similarly here, 'weight' is a scalar and not an array.

TranslateFormBase is the only one I'm not immediately sure about.

@longwave , i got your point and i have checked in core only TranslateFormBase needs replacement in this case IMO.

There'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 new InputBag
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, because has() could fire for empty array passed in
- \Drupal\media_library\MediaLibraryState (getAllowedTypeIds() and getOpenerParameters() as class extends ParameterBag we can skip modify this methods but in fromRequest() method there's $state->replace($query->all()); which could be incompatible with InputBag from query string

The interdiff as against #4

+++ b/core/modules/locale/src/Form/TranslateFormBase.php
@@ -131,7 +131,7 @@ protected function translateFilterValues($reset = FALSE) {
-        $value = $this->getRequest()->query->get($key);
+        $value = $this->getRequest()->query->all()[$key];
         if (!isset($filter['options']) || isset($filter['options'][$value])) {
           $filter_values[$key] = $value;

No reason to change it, the $value used as array key so it must be string

+++ b/core/modules/field_ui/src/Form/FieldConfigEditForm.php
@@ -206,7 +206,7 @@ public function save(array $form, FormStateInterface $form_state) {
-    if (($destinations = $request->query->get('destinations')) && $next_destination = FieldUI::getNextDestination($destinations)) {
+    if (($destinations = ($request->query->all()['destinations'] ?? [])) && $next_destination = FieldUI::getNextDestination($destinations)) {

+++ b/core/modules/field_ui/src/Form/FieldStorageConfigEditForm.php
@@ -224,7 +224,7 @@ public function save(array $form, FormStateInterface $form_state) {
-      if (($destinations = $request->query->get('destinations')) && $next_destination = FieldUI::getNextDestination($destinations)) {
+      if (($destinations = ($request->query->all()['destinations'] ?? [])) && $next_destination = FieldUI::getNextDestination($destinations)) {

+++ b/core/modules/jsonapi/src/EventSubscriber/ResourceResponseSubscriber.php
@@ -144,10 +144,10 @@ protected static function generateContext(Request $request) {
-    if ($request->query->get('fields')) {
+    if ($fields = ($request->query->all()['fields'] ?? [])) {

+++ b/core/modules/menu_ui/src/Controller/MenuController.php
@@ -50,7 +50,7 @@ public static function create(ContainerInterface $container) {
-    if ($menus = $request->request->get('menus')) {
+    if ($menus = ($request->request->all()['menus'] ?? [])) {

extra wrapping parentheses () could be skipped but I've added them for readability

+++ b/core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php
@@ -128,7 +128,7 @@ public function processAttachments(AttachmentsInterface $response) {
-    $ajax_page_state = $request->request->get('ajax_page_state');
+    $ajax_page_state = $request->request->all()['ajax_page_state'] ?? [];

As 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.

Or 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?

@longwave Thanks for review, missed the point that default for get() is NULL so changed back that hardening and now broken tests passed.

As of all() it's since SF 2.x for ParamerterBag() 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

What 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

    $ajax_page_state = $request->request->all()['ajax_page_state'] ?? NULL;

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.

    $ajax_page_state = $request->request->all('ajax_page_state');

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.

Cleaner title and fix tag :)

Yeah 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?

So 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.

This 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.

I 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.

Discussed with @catch - we agreed to postpone on #3198594: Forward compatibility layer for InputBag

Dependency commited

This can now use the new argument to all() where appropriate.

Opened 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.

Fixed 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.

needs to fix cspell and use

Fixed the use statements.

NW for the last two failures.

I think this should be green now.

I 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.

Addressed #39, also reworked MediaLibraryState so it can convert ParameterBags by itself the same way the kernel does, which avoids changing the test.

Addressed @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.

Thanks 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 :)

I find changes to empty() from isset() strange, or default values require it now?

@andypost all() always returns an array, isset() on any array (even empty) is true, so we need to use empty() instead.

@andypost we could change many of those isset/empty() checks to ->query->has() which would make it cleaner?

@longwave great idea! Surely much clearer

NW for #45-47, will try to get to this this week

Changed 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?

The fail is unfortunately not random.

Thanks for reviews, feedback addressed.

The remarks of @alexpott have been addressed.
I have updated the CR.
For me it is RTBC.

All Symfony 6 issues are critical.

Committed and pushed 8b8fc1eff7 to 9.3.x and 8f0f57f3e5 to 9.2.x. Thanks!

This 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.

#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.

Re #60 Isn't this a great opportunity to test them as well? Or it is out of scope?

@huzooka we definitely need to add test coverage of what broke the contrib module.

I 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.

So maybe we can do something like this, as long as others use Request::create() and not new Request() I think this will work.

Oh, except we already override the request factory in DrupalKernel::setupTrustedHosts().

#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?

Addressed #67. The TrustedHostsRequestFactory logic is only triggered if trusted hosts are set up, so I extended the existing functional test for this case.

The patch looks good. Just one remark:

+++ b/core/modules/system/tests/src/Functional/System/TrustedHostsTest.php
@@ -111,4 +111,26 @@ public function testShortcut() {
+    $host = $this->container->get('request_stack')->getCurrentRequest()->getHost();
+    $settings['settings']['trusted_host_patterns'] = (object) [
+      'value' => ['^' . preg_quote($host) . '$'],
+      'required' => TRUE,
+    ];
+
+    $this->writeSettings($settings);

Why is this added to the test. I think we can remove it.

See #71. TrustedHostsRequestFactory is only initialised at all if a host pattern is set, but by default it is empty. See DrupalKernel::initializeSettings():

    $host_patterns = Settings::get('trusted_host_patterns', []);
    if (PHP_SAPI !== 'cli' && !empty($host_patterns)) {
      if (static::setupTrustedHosts($request, $host_patterns) === FALSE) {

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?

@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.

The 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.

@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.

Note to committers: the merge request was the reverted version of this code - the patch in #71 is the updated version with the subrequest fix.

There is a subtle bug in #71 actually:

+++ b/core/lib/Drupal/Core/Http/TrustedHostsRequestFactory.php
@@ -61,7 +61,17 @@ public function createRequest(array $query = [], array $request = [], array $att
+      if (!($bag instanceof SymfonyInputBag)) {

This class needs a use statement.

Use-statement added.
Back to RTBC.

Added @todos.

I bet both todos need links to get rid of them