Request/response should be injected in controller, actions and related classes via constructor

[samdark]

Current state

Currently we're working with request/response globally via Yii::$app->request and Yii::$app->response.

Problem

Encapsulation violated. Request and response aren't meant to be dealt with outside of controller and related classes. Accessing them directly and especially doing that in the guide could be easily interpreted like "accessing request anywhere via Yii::$app->request is OK".

Suggested syntax

// old
Yii::$app->request->...
Yii::$app->response->...

// new
$this->request->...
$this->response->...

[Alex-Bond]

How about PSR implementation?)

[blacksmoke26]

+1 @samdark

[samdark]

@Alex-Bond PSR-7 or PSR-15?

[Alex-Bond]

@samdark PSR-7 for start. And PSR-7 more closer to this issue i think.

[allowing]

Yii::$app并没有违反封装性。

从DI里获取实例，需要先访问到DI实例，如果DI实例没有用一个全局的东西（函数、类的静态属性）引用着，那么就须要一级一级地在函数调用中传递它，现在有了Yii::$app这个全局的属性，它引用这DI实例。这个有点像 Laravel 的 Facade 。

当然，在非 Yii 应用里，如果使用了 Yii::$app 就确实违反了封装的原则。

---

既然 Yii 已经用反射实现了依赖的解析，那么完全可以在调用任何方法时，去反射调用这个方法，那么这样子，程序员就可以在他的方法中用类型提示声明自己的依赖。

[samdark]

Translation via Google Translate:

Yii :: $ app does not violate encapsulation.

To get an instance from DI, you need to access the DI instance first. If the DI instance does not reference a global thing (a static attribute of a function, a class), it is necessary to pass it once in a function call. The Yii :: $ app this global attribute, which references the DI instance. This is a bit like Laravel Facade.

Of course, in a non-Yii application, the use of Yii :: $ app does violate the principle of encapsulation.

Since Yii has implemented dependency analysis with reflection, it is entirely possible to invoke this method when calling any method, so that the programmer can declare his dependency with a type prompt in his method.

[cebe]

We had this already implemented in an early stage of Yii2 development, I do not remember the exact reasons why we dropped it: https://github.com/yiisoft/yii2/commit/b138199c59c0547429f62eb106e7b5d6c251f9cd

[samdark]

https://github.com/yiisoft/yii2/pull/548

[cebe]

I found the original discussion about reverting it. Here are some points to discuss:

• Error Handler needs to work on response but it works on application level, not controller level. "for example, in a Web API action, how to generate a JSON error response?"
• PageCache needs to work with request and response ( this will probably work fine but should be checked)
• beforeAction and afterAction need access to the response and request object.
• implementation in base\Controller should be independent of console\Controller and web\Controller
• a response object on app can be configured in application config, with default headers or something, this is not as easily possible if a controller has its own response.
• implement content format negotiation based on Accept Type header, with app response very easy as the format will be set on app component once and not for each controller
• "what if a widget wants to do redirection?"

[cebe]

here is the revert commit: https://github.com/yiisoft/yii2/commit/f18d6df625b7e977c278ff097e50effc670b7c73

[samdark]

Error Handler needs to work on response but it works on application level, not controller level. "for example, in a Web API action, how to generate a JSON error response?"

Application may accept an object of response to send it. What forms it doesn't matter. Thus error handler may create a new response instance and send it to the application to render it.

PageCache needs to work with request and response ( this will probably work fine but should be checked)

It's fine. Request/response could be obtained via $this->owner->request and $this->owner->response.

beforeAction and afterAction need access to the response and request object.

Should be fine. I see no reason why these won't be available.

implementation in base\Controller should be independent of console\Controller and web\Controller

Should be OK.

a response object on app can be configured in application config, with default headers or something, this is not as easily possible if a controller has its own response.

Yii::createObject() solves the issue. Configuration for defaults goes to DI container.

implement content format negotiation based on Accept Type header, with app response very easy as the format will be set on app component once and not for each controller

Setting it for each controller is basically the same as setting it for an application since controller is executed once in both cases.

"what if a widget wants to do redirection?"

Yes, that could be a problem.

[brandonkelly]

Request and response aren't meant to be dealt with outside of controller and related classes. Accessing them directly and especially doing that in the guide could be easily interpreted like "accessing request anywhere via Yii::$app->request is OK".

Why is that so bad? Yii is a web application framework. There is only one (genuine) HTTP/console Request and Response per application instance. And the bootstrap file doesn’t run a Controller, it runs the Application. So if some component of the Application, or a helper class, needs to find out something about the current request, or modify the response, that seems perfectly reasonable to me.

One issue that hasn’t been brought up: what about all the times the Request or Response are needed before a controller has been run?

For example, Yii itself calls getRequest() several times leading up to runAction():

• when setting @webroot and @web aliases (web\Application::bootstrap())
• when determining whether the client has an active PHP session (Session::getHasSessionId())
• when handling the actual request (web\Application::handleRequest(), console\Application::handleRequest())

These things are being done an the application level, not as a function of a controller. And being a web application, that’s OK.

Craft CMS has several places where it also needs to work with the request, before a controller has been run. Here are a few examples that I don’t believe will be easy to fix if this change is implemented:

• when configuring the log component (config/app/main.php)
• when configuring the user component on web requests (config/app/web.php)
• when determining the target language (ApplicationTrait::_setLanguage())
• when determining whether the Debug extension should be enabled (bootstrap/bootstrap.php)

[brandonkelly]

I do agree that it would be good for controllers to have a direct reference to the Request and Response objects that they are concerned with, in the same way that it is good for DB migrations to have a reference to the Connection object.

Why not keep Yii::$app->getRequest() and Yii::$app->getResponse(), to get the main application Request and Response objects, but add public $request and $response properties to yii\base\Controller:

class Controller extends Component implements ViewContextInterface
{
    public $request = 'request';
    public $response = 'response';

    public function init()
    {
        parent::init();
        $this->request = Instance::ensure($this->request, Request::className());
        $this->response = Instance::ensure($this->response, Response::className());
    }

    // ...
}

(Or better yet, replace Request::className() and Response::className() with PSR-7 RequestInterface and ResponseInterface.)

That way applications can still reference the main Request/Response objects when they need it, but Controllers can always reference the specific Request/Response objects they should be concerned with, via $this->request and $this->response.

[mikk150]

Well I was just looking for this pretty much(not exacly, but oh well)

I want to be able to run multiple yii\base\Request objects in one application (basically queue worker)

Maybe this could be my way to do it...

[cebe]

@mikk150 could you explain more about why you want multiple requests and Responses in one application? an application ususally handles one HTTP request so if there are other use cases I am interested in the details of these.

[samdark]

@cebe not sure what scenario @mikk150 has but I think it could be something about running a web server such as http://www.swoole.com/ that initializes framework once and then handles requests/responses.

[mikk150]

@samdark good reference, I did not know it existed, but no... @cebe what if I told you that Yii2 does not support only web applications? but console applications... What If I told you that you could get requests from pool and proccess them as queue workers.....

[cebe]

What If I told you that you could get requests from pool and proccess them as queue workers.....

You already told me and I'd like to hear more about how that would work ;)

[mikk150]

Rewrite \yii\base\Application::run() so it wouldn't automatically start \yii\base\Application::handleRequest() but rather set up an loop to make new \yii\base\Request object each time a job has been found in queue

Having said that, I could do it right now :)

My worker runs in Thread inside an Worker which I could use to hold this application which is inside Pool of [n] workers

So there is one main application which job is to take jobs from queue and encapsulate them into Threads and push them to Pool for Workers to work on

[klimov-paul]

it could be something about running a web server such as http://www.swoole.com/ that initializes framework once and then handles requests/responses.

Even while creaing your own web server using PHP there is always a global request and response for the script. While launching a PHP deamon you still passes shell parameters to it. Even PHP-based web server has its own specific global request (shell arguments) and its own specific global response (stdout).

[klimov-paul]

The global application references for request and response can not be just removed. However we can introduce additional controller methods: getRequest() and getResponse(), which by default may serve as a simple shortcuts to Application::getRequest() and Application::getResponse().

However even so there is many places, where such classes as models and widgets may use Request data. For example:

1) BlameableBehavior relies on gloabl yii\web\User state, which depends on web session, which is actually a part of web request: https://github.com/yiisoft/yii2/blob/master/framework/behaviors/BlameableBehavior.php#L100

2) GridView uses a Url helper to compose filter URL, while Url helper depends on UrlManager, which is also dependent on web request: actual domain name, request URI and params vary on request: https://github.com/yiisoft/yii2/blob/master/framework/grid/GridView.php#L334

[klimov-paul]

it could be something about running a web server such as http://www.swoole.com/ that initializes framework once and then handles requests/responses.

If I would create such solution I would better switch or update global Application::$request and Application::$response instead of endless passing of these objects over all class, which may need it.

[mikk150]

But then this issue is invalid?

[samdark]

It's not decided yet. Paul arguments are quite strong though.

[klimov-paul]

But then this issue is invalid?

Why is that? You can use following code to replace current request/response components:

Yii::$app->set('request', ['class' => \yii\web\Request::class]);
Yii::$app->set('response', ['class' => \yii\web\Response::class]);

[mikk150]

well, yes, ofc I can, and I will I think at least

It's about this task

[Faryshta]

@klimov-paul agree, at the very least those helper methods won't be able to remain statics anymore.

Url::to(...) would become something like

$urlHelper = new UrlHelper($request->getRoute());
$urlHelper->to(..);

[mikk150]

UrlHelper could stay as is, because router is in application, and you could take router from controller etc, because controller belongs to application, but one application can run multiple controllers

edit:// oh yeah, you would need to put request into it...

[Faryshta]

lets say i me a request to route `module/controller/action'

if I run Url::to(['index']) will return module/controller/index since the router will write it based on my original request.

but if i request the route controller/action and run the exact same code would return controller/index.

thats why it will have to be rewritten as instantiated methods

[mikk150]

but if you write

Url::to($request, ['myaction'])

Now URL knows what is going on and you do not need dependency injection to feed urlManager in

[Faryshta]

@mikk150 that would be extremely unefficient when dealing with several calls like for example a menu with several links.

what if a widget wants to do redirection?

Yes, that could be a problem.

$widget->view->controller->response or a short cut with a getter. which can also help with the helpers issue

$widget->view->getUrlHelper() or something like that which handles url creation using the request associated to the controller.

[SamMousa]

Some (possibly) related thoughts:

I'm using component injection (something that didn't make it in core, I implemented it via a trait: https://github.com/SAM-IT/yii2-magic/blob/master/src/Traits/ActionInjectionTrait.php)

The reasoning here is that a controller does not necessarily have a dependency on Request or any other application component. It might use them but to pass them in in the constructor is a bit overkill. Ideally if you have a separate action class you'd inject them in that constructor.

Regarding the global response object, there has been a lot written about this (as pros and cons of PSR-7). One often cited issue is for example that there is state pollution (already we see for example that the ErrorHandler tries to take the response object and bring it in to a usable state).

As much as possible I try to abstract my application code away from using global request / response / session objects. Instead I create wrappers that I can inject, for example:

<?php

namespace app\components;

use yii\web\Session;

class NotificationService extends \yii\base\Component
{
    private $_session;
    private $_counter = 0;

    public function __construct(Session $session, array $config = [])
    {
        parent::__construct($config);
        $this->_session = $session;
    }

    public function success($message, $category = null)
    {
        $this->_session->setFlash(
            $category ?? "Message" . $this->_counter++,
            [
                'type' => \kartik\widgets\Growl::TYPE_SUCCESS,
                'text' => $message,
                'icon' => 'glyphicon glyphicon-ok'
            ]
        );
    }

    public function error($message, $category = null)
    {
        $this->_session->setFlash(
            $category ?? "Message" . $this->_counter++,
            [
                'type' => \kartik\widgets\Growl::TYPE_DANGER,
                'text' => $message,
                'icon' => 'glyphicon glyphicon-ok'
            ]
        );
    }

}

Advantage of this approach is that my controller / action dependencies are limited to specific use cases. I think this could work very well for the mentioned use case of widgets wanting to do a redirect. You'd be able to have lightweight services and get code like:

$widget->navigationService->redirect()

These services would then be injected.

Another advantage is that the dependencies are a lot clearer and more specific. Notifications are a good example, they often depend on the Session but there is no technical reason why they should. Therefore it makes sense to abstract it.

The same approach could work for the UrlHelper; it is a strange helper anyway, since it depends on a lot of global state. Instead you would get something like an UrlService that could be injected when needed (and probably by default in views).

[Faryshta]

as it was mentioned in #14064 i think its also worth to detach the components user, view, controller and others from Yii::$app.

My idea is

• yii\base\Application creates the $request object after figuring out the requested route using Yii::$app->urlManager and attach it to DI.
• $request uses the route to create $request->controller. alternatively it can create also $request->user
• $controller = $request->controller has a getter getRequest() which returns the request that created it.
• $controller creates an object $response and attach it to DI.
• $controller creates $controller->action object which can get passed any object already attached to DI.
• $controller->action->run() method will return an instance of yii\base\Response or data to pass to the object $response which was already attached it to DI.
• that $response object might contain a $response->view object.
• the $view object gets attached the $request and $response object to make them accessible on view files using getters.

/** @var $this yii\web\View */
echo $this->request->user->username;

gladly DI can help a lot for example

DefaultController
{
    public function action(int $id, Request $request, Response $response, IdentityClass $user): Response
    {
        // magic
        return $response->renderView($params);
    }
}

[SamMousa]

I think an injected response Factory would be better.

On May 3, 2017 08:57, "Angel Guevara" notifications@github.com wrote:

as it was mentioned in #14064 https://github.com/yiisoft/yii2/issues/14064 i think its also worth to detach the components user, view, controller and others from Yii::$app.

My idea is

• yii\base\Application creates the $request object after figuring out the requested route using Yii::$app->urlManager and attach it to DI.
• $request uses the route to create $request->controller. alternatively it can create also $request->user
• $controller = $request->controller as a getter getRequest().
• $controller creates an object $response and attach it to DI.
• $controller creates $controller->action object which can get passed any object already created using DI.
• $controller->action->run() method will return an instance of yii\base\Response or data to pass to the object $response which was already attached it to DI.
• that $response object might contain a $response->view object.
• the $view object gets attached the $request and $response object to make them accessible on view files

/* @var $this yii\web\View /echo $this->request->user->username;

gladly DI can help a lot for example

DefaultController{ public function action(int $id, Request $request, Response $response, IdentityClass $user): Response { // magic return $response->renderView($params); }}

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/yiisoft/yii2/issues/13922#issuecomment-298835950, or mute the thread https://github.com/notifications/unsubscribe-auth/AAhYzSp4EILqy_mq8iJxFsq3F9_7LEovks5r2CVmgaJpZM4MziTy .

[mikk150]

@Faryshta \yii\base\Application should not create \yii\base\Request object Go read https://github.com/yiisoft/yii2/issues/13922#issuecomment-297186909 and see why you should not do this

And factories.. I do not know.. Yii has never been strong on factories

[Faryshta]

@mikk150 what you say about loops and queue is also not mutually exclusive with what i said either.

specially since Yii::$app can get configured to contain the initialization logic for the request objects such as the class it will instantiate.

[mikk150]

How, if application starts parsing the request? Maybe I do not have the request at application initialization? it comes in later

[Faryshta]

i never said application creates the request after initialization. request is created after application determines the route.

which as you said might come later

[klimov-paul]

Solution proposed at #14645

[SamMousa]

I'm not a fan of the proposed solution; it still breaks encapsulation inside the framework... I have to admit though, it would reduce breaking encapsulation in application code...

For things that are as common as Controllers and Actions I do not see the downside of using factories though. Pass a ControllerFactory to each Module which passes an ActionFactory to each controller.

These factories can then have and inject a reference to the Request, and may inject a Response object... However this should be a fresh response object that an action can use, the current approach of using a global one and randomly cleaning it / modifying it seems a bit unclear...

For example:

function actionTest() {
    $this->response->format = 'json';
    return '{}';
}

Does not feel clean to me at all; setting something on the response object is a weird method of changing the output of the application.

function actionTest(): Response {
    // This uses the injected ResponseFactory to create a new response object.
    $response = $this->createResponse();
    $response->format = 'json';
    $response->content (or is it data?) = '{}';
    return $response;
}

This is much clearer in my opinion, we still need to figure out how filters handle this...

[SamMousa]

http://docs.php-http.org/en/latest/message/message-factory.html

https://github.com/php-fig/fig-standards/blob/master/proposed/http-factory/http-factory.md

[klimov-paul]

14645 is the only thing I can do at 2.0.x without BC break.

Any further adjusments can be done only at 2.1.

[klimov-paul]

These factories can then have and inject a reference to the Request, and may inject a Response object...

Binding request and response to a controller has nothing to do with the dependency injection. It is the matter of controller handling the particular request assigned to it. By the time controller is instantiated, Request object is already exists and determined. In fact, Request instance actually determines, which controller will be created to handle it through the routing mechanism.

Processing of the controller response might be different. On one hand, it may seem logical for the controller to create a response object itself, on the other hand it may produce troubles with filtering feature. Filters attached to the application or module level may adjust the response object, which then should be passed to controller. I can not see any problem application determining the response instance to be used by controller.

Also note: even at present state, controller is able to create its own Response instance and return it as its result. In this case this instance will be used as application response despite the value of Application::$response:

class FooController extends \yii\web\Controller
{
    public function actionFoo()
    {
        return new \yii\web\Response([
            'format' => 'json',
            'data' => ['some' => 'test']
        ]);
    }
}

For the single request/response application such as yii\web\Application having single request and response instances is absolutely fine. Introduction of special factory for the response objects is just a redundant overcomplication of the code. Such solution makes sense only for some deamon-kind application, which performs multiple Application::run() calls or processes multiple requests in the chain. Such application can be developed with the fix provided by #14645 as particular controller can be populated with particular request and response, which may be different from the ones used by main application.

[samdark]

Need to deal with mutable headers collection https://github.com/yiisoft/yii2/pull/14701/files/304ca359dbaf6d47e562f662752940a55cbfcc69#r134578641

[klimov-paul]

I am against of passing Request/Response to the controller via constructor, as reduces flexibility as same controller/action can be re-used for processing different requests.

Consider deamon-like application, which is stored in the memory processing multiple incoming requests. While such application creates controllers by demand via some routing mechanism, it may keep created controllers in memory, re-using them in case another request matching its route appear.

Strict binding Requst/Response to the controller will make this controller to be single-use object. There should be ability to re-set request and response for existing controller intance.

[rob006]

@klimov-paul Why do you want to reuse controller instance?

[klimov-paul]

I can re-use it inside the deamon application. There is no need to re-create controller instance per each request while running memory resident application. Deamon may store already created controllers and just re-run thier action methods - such approach will save performance for it.

[klimov-paul]

In fact common deamon application, which pocesses some queue items may actually have a single controller with single action and use it for every incoming requests, which are created from queue item data. It would be very uneffective, if in such flow program re-creates controller object per each request.

[samdark]

While using PHP 7 it doesn't make a big difference in resources and time consumption to create a single class per request. Even in case of daemon it will guarantee that there will be no side effects and no stuck values.

[klimov-paul]

If there is no difference for new object creation, why do we bother with controller to have its own request/response? In this case it is better to re-set request and response at application level as I have suggetsed here: https://github.com/yiisoft/yii2/issues/13922#issuecomment-297377802

And then simply create a shortcut methods getRequest() and getResponse() inside the Controller, which will simply invoke Yii::$app->getRequest() and Yii::$app->getResponse().