On the one hand: Using Request::get() is almost always a bad idea, so deprecating it will help me maintain my sanity as a code reviewer in the future.

On the other hand: Migrating a mature codebase away from that method is a huge PITA. Been there, done that, won't recommend. 🙈

what i like about this deprecation is it moves decision taking to the user's side (aka the "use instead" part)

A) i'll use the correct input source explicitly, B) i'll inline get() into a one-liner using nested $default args

Both acceptable to me, preferring A most likely :) overall i think it's worth forcing users to make the intentions more expressive.

maybe an interim step could also be to deprecate accessing POST in that function. i.e. add a deprecation notice here:

Because like I said, I don't see how you could use this safely for POST forms

The method starts with the following disclaimer:

 * This method is mainly useful for libraries that want to provide some flexibility. If you don't need the
 * flexibility in controllers, it is better to explicitly get request parameters from the appropriate
 * public property instead (attributes, query, request).

And looking at the codebase, this method is exactly used for that: fetching a value either in the route, query or body.

Before considering deprecating, we should ensure that these use cases can be updated and that the change would make sense.

@nicolas-grekas Well the thing is that the disclaimer is wrong IMO: It doesn't provide flexibility, it just introduces uncertainty. Because making POST data overrideable by GET just opens you to all sorts of trouble from external parties

Please have a look at the codebase. The places where get() is used look aligned with the disclaimer to me.

I don't doubt that it is used correctly in this repo, I'm more worried about external code of varying quality. Who's to say that every dev using this function read and fully understood the disclaimer? The point is that it's just very easy to get it wrong and introduce vulnerabilities, so to me at least the function seems badly designed. If you want to keep it for use in this repo, maybe tag it as @internal, otherwise I think the disclaimer needs to be a lot stronger than it is now. E.g. something like this could be added:

 * Do not use this function to read data from POST forms! Doing so allows third parties 
 * to override the users' choices in the form by sending them specially crafted URLs.

It's weird that I have to keep repeating it, but to me this function seems like a security incident waiting to happen. Is nobody else worried about the abuse potential here? Am I missing something?

I would welcome a PR that removes using the method from the codebase. That could be a nice exercise to practice the proposal and get some more food for thoughts.

out of curiosity, I added xdebug_print_function_stack to the top of the get function and ran the test suite. It showed the following call sites:

src/Symfony/Component/HttpFoundation/Request.php:464
src/Symfony/Component/HttpKernel/Fragment/InlineFragmentRenderer.php:129
src/Symfony/Component/HttpKernel/Fragment/InlineFragmentRenderer.php:130
src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php:105
src/Symfony/Component/Security/Http/ParameterBagUtils.php:75
src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php:89
src/Symfony/Component/Security/Http/Firewall/AbstractListener.php:26
src/Symfony/Component/Security/Http/HttpUtils.php:101
src/Symfony/Component/Security/Http/ParameterBagUtils.php:80

(take that list with a grain of salt though: I get tons of errors & failures when running the test suite)

Side question: does anyone know a static anaylzer that can find all call sites for a function? I'm looking for this for some other project, and it might come in handy here, too

Side question: does anyone know a static anaylzer that can find all call sites for a function?

PhpStorm? 🙃

Please consider different naming otherwise. We have +- 60 calls, and im totally not amused about it :D

Plus one for deprecating that function. I agree that conflating get/post params can lead to subtle bugs, often with security implications, and should not be something so easily available to end developers. What's more, I always found it name confusing, as in my mind get() is too close to $_GET

aside from being overly long, I think a good name for this function would be getAttributeOrQueryOrRequest: It tells you exactly what it does and also kind of conveys why it's usually a bad idea to use it.