Removing Controller::getRequest() deprecation note

[weaverryan]

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	--
License	MIT
Doc PR	not yet 👼

Hi guys!

When the request_stack was introduced, we deprecated Controller::getRequest() - #9553.

I would argue that there is value to this method, as it saves me from needing to add a use statement in my controller to get the request. This is similar to why we have the redirect shortcut method.

Since the method does use the request_stack, I don't see the disadvantage to having this.

Thanks!

[fabpot]

It was also deprecated as you can inject it into your actions if needed. Having two ways of doing the same thing is never a good idea (and injecting the request works with any controllers.) That said I do understand the argument of the extra use statement.

[linaori]

I know programmers are supposed to be lazy, but adding a use statement is not a problem unless you write your code in notepad.exe. In my opinion, it should not be a reason to remove this deprecation.

[shoomyth]

If you are lazy you can create your own base controller class and add getRequest() method there :)

[weaverryan]

Hi guys!

I do occasionally see Notepad++ actually - especially with beginners ;). I also don't expect a beginner to add their own base controller method - if it's useful, it should just be there.

The only disadvantage I see is what Fabien mention: that there would be 2 ways to get the request. In the Framework, I would actually be ok with always showing getRequest() in the docs - it's more consistent with how we get any other object. A small mention would remain (as well as plenty of docs on the web) about using it as an argument if you're registering your controller as a service.

So, I'd like to un-deprecate this and actually recommend it in the docs.

Thanks!

[linaori]

@weaverryan If anything, I like Symfony to recommend nice solutions over the easy ones. Yet it doesn't mean that Symfony shouldn't provide the easy options nor ignore them in the documentation.

CoaS vs Parent Controller
With 1 method, the amount of lines in your class (not counting constructor) are equal. But for every method you add to your controller, you get 1 more line. This because you only need to add 1 use statement but $request = $this->getRequest() is per method, unless you want to call getRequest() every single time and that's a bad design in my opinion.

I really, really don't want to see this un-deprecated because of the argument that a use-statement is too much work. I think it's fine the way it is and the current solution works for both Controller as a Service and with a parent controller.

// DIC/service wise
use ...;
//...
public function someAction(Request $request)
{
    return [
        'address' => $this->addressService->getFromRequest($request),
        'contact' => $this->contactService->getFromRequest($request)
    ];
}

// Parent Controller wise
public function someAction()
{
    $request = $this->getRequest();
    return [
        'address' => $this->get('address_finder')->getFromRequest($request),
        'contact' => $this->get('contact_finder')->getFromRequest($request)
    ];
}

PS: if anything, Symfony2 should recommend Controller as a Service in the docs because it enforces 3 important things:

• Fail as early as possible. If a dependency is missing, it will fail at compile time rather than when you trigger an edge case in production
• Thinking about your dependencies, using get() may easily lead to too many dependencies too fast
• Using the container directly is never a good idea and kills the whole idea behind it. In my opinion, your application may not even know about the Container at run time

[javiereguiluz]

I agree with Ryan here.

I know that experienced Symfony developers won't use this ... but unfortunately, for the regular PHP developer, adding a use ... statement is too hard most of the time.

[weaverryan]

And actually, I would use this - it saves me from going to the top of my method or file to add other information. That's a small convenience, but I would prefer this way :).

[wouterj]

1. Yes, 👍 for removing the deprecation note. I can see why it's easier
2. No, 👎 for making this the recommended way in the documentation. The documentation could mention it, but as the typehinting works for both controllers as services and normal controllers and the shortcut method doesn't, I would still use the typehinting in the docs. (otherwise, we're promoting to use the base controller when creating controllers as services even more).

[hhamon]

I'm 👎 for removing the deprecation. I think it's important to force newcomers to understand the big picture of Symfony: converting a request into a response. And that's what a controller does in Symfony. There is no magic! It makes also more sense to remove this getRequest method in 3.0 as there is no getResponse method in the base controller class.

In my opinion, if the developer forgets to put the use statement at the top of its class to inject the request object into the controller, I'm pretty sure we can improve the autoloader exception message to something like this:

It seems you are trying to inject the Symfony request object into your
FooBundle:Bar:qux controller but the autoloader cannot find the corresponding class.
Did you forget to use the Symfony\Component\HttpFoundation\Request class namespace at the top of your BarController class?

We can easily deduct the use statement is missing if the typehint of the argument is Request. In 99% of cases, it will be the Symfony request object.

[stof]

@hhamon AFAIK, we already suggest that you forgot the use statement and suggest alternatives when using the DebugClassLoader

[weaverryan]

I do agree with Hugo's point on how having the Request as an argument adds clarity to the "your job is to transform a request into a response" mantra. So, I'm going to close this.

BUT, if generators add the use statement and beginner docs that talk about controllers includes the use statement, then we probably won't have this problem anyways. I'll follow up in that direction.

Thanks for the conversation