Remove the usage of `Request::get`

[franmomu]

Feature Request

It has been marked as @internal in Symfony 5.4 and its usage is confusing, we should call explicitly the proper input.

[VincentLanglet]

Even if we're only using the query for an input. Changing, for instance,

$request->get('foo')

to

$request->query->get('foo')

is a BC-break since if the user were passing the value in the attributes or the request... So when should we do it ?

Also we are generally setting the data in multiple places. As an example, _sonata_admin is often in the attributes (like when we're on an Admin page), but we sometimes pass it in the query parameter (like when we're calling AppendField). Should we do something like

$request->attributes->get('_sonata_admin', $request->query->get('_sonata_admin'))

or can we always use the same input ? If so, changing the input can also be considered as BC-break.

[simonberger]

I would say it can still be valid to ask the attribute, query and request bags without using the Symfony function, but in best case it is placed (early) in a controller where it is known which method is used.

[VincentLanglet]

The following issue https://github.com/sonata-project/SonataAdminBundle/issues/3062 could help. Restricting most route for GET will reduce request usage to query and sometimes attributes.

[kirya-dev]

Some optimized:

$request->attributes->get('_sonata_admin') ?: $request->query->get('_sonata_admin')

[VincentLanglet]

_sonata_admin is used

• as attribute for lot of routes
• as query params for some routes like appendFormFieldElement or getShortObjectDescription
• as request param in RetrieveAutocompleteItems

In the same way, all the data in retrieve autocomplete items action seems to be passed in the request: _context, field, q, ... And I wonder if the $request->query->get($reqParamPageNumber, '1') is not wrong. Also, it's seems to be the only route where data is passed in the request instead of the query. Even appendFormFieldElement or SetObjectFieldValue or RetrieveFormFieldElement which are made by POST are passing all the data in the query.

In the controller, $request->get($this->admin->getIdParameter()); could be replaced by $request->attribute->get($this->admin->getIdParameter()); and calls like $request->get('btn_preview') could use $request->request instead.

Also, the https://github.com/sonata-project/SonataAdminBundle/issues/7185 issue is showing that sometimes id or parentId are passed as query params instead of attributes so in the AbtractAdmin $this->getRequest()->get($parentAdmin->getIdParameter()) should check the attributes AND the query params.

I find over 60 usage of the get method, and most of them will need at least attributes->get(...) ?? query->get(...) or even attributes->get(...) ?? query->get(...) ?? request->get(...) ; so we won't gain a lot over the usage of this internal method.

Did you plan to make a poc @franmomu ? Should we really try to stop using the internal method ? Should we implement a static utils method then which does the same ? I don't think it's great to duplicate the symfony method...

Any thought @jordisala1991 ?

[jordisala1991]

It would be nice to change all get to what is needed on 4.x, even if it needs a lot of code.

The idea is to reduce that for 5.x if it is possible.

So, maybe now we need to fetch a param from 3 places of the request, but in next major we can refactor the code to only be from one place.

[VincentLanglet]

It seems to be that we wont be able to reduce the usage of some of them (which arr frequently used) like _sonata_admin or the idParameters like explained in the previous post.

Maybe only half of the get calls can be restricted to the query/request/attributes. But that would require lot of work for small/none gain...

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.