Fix #15438 PSR middleware #15459

klimov-paul · 2018-01-05T11:07:01Z

Q	A
Is bugfix?	no
New feature?	yes
Breaks BC?	yes
Tests pass?	yes
Fixed issues	#15438, #10659

…optional

SilverFire · 2018-01-06T08:14:32Z

framework/base/Controller.php

@@ -186,12 +213,12 @@ public function run($route, $params = [])
    {
        $pos = strpos($route, '/');
        if ($pos === false) {
-            return $this->runAction($route, $params);
+            return $this->runAction(Yii::$app->getRequest(), $route, $params);


We had a discussion about injecting Request instance in controller. Seems to be useful in this case

The only way to do this in the middleware scope is having a setter for the request instance allowing request to be re-set per each action execution in the same way Controller::$action behaves.
Such approach was rejected at #14645.
This matter should be solved afterwards.

SilverFire · 2018-01-06T08:15:47Z

framework/base/Module.php

        if (strpos($route, '/') !== false) {
-            [$id, $route] = explode('/', $route, 2);
+            list($id, $route) = explode('/', $route, 2);


Not sure.
Should be fixed.

SilverFire · 2018-01-06T08:18:33Z

Why do we have (nearly) the same Middleware-related code in yii\base and yii\http\server?

SilverFire

Great work. To be polished

klimov-paul · 2018-01-06T12:24:06Z

Why do we have (nearly) the same Middleware-related code in yii\base and yii\http\server?

Middleware at 'yii\http\server' is bound to the PSR 15 interfaces and use them for type-restrictions. Attempt of usage for the Interop\Http\Server\MiddlewareInterface instance at yii\base\MiddlewareDispatcher will end with PHP error.
Still if anyone has a good idea - be my guest.

samdark

Dispatcher looks good. I think that defining middleware stack should not be tied to modules/controllers but instead be done at router level.

samdark · 2018-01-07T21:12:14Z

framework/base/Controller.php

     * @param string $id the ID of the action to be executed.
     * @param array $params the parameters (name-value pairs) to be passed to the action.
     * @return mixed the result of the action.
     * @throws InvalidRouteException if the requested action ID cannot be resolved into an action successfully.
     * @see createAction()
     */
-    public function runAction($id, $params = [])
+    public function runAction($request, $id, $params = [])


Could be type-hinted with Request.

samdark · 2018-01-07T21:16:38Z

framework/base/Controller.php

+     * @see MiddlewareDispatcherInterface::dispatch()
+     * @since 2.1.0
+     */
+    public $middleware = [];


Middleware is meant to be set on the router/url manager config level so it could be applied to a number of routes some of which could be non-controllers. It's not meant to be part of the controller itself although previously filters were controller-only thing.

samdark · 2018-01-07T21:17:11Z

framework/CHANGELOG.md

@@ -22,6 +22,8 @@ Yii Framework 2 Change Log
 - Enh #13702: Added support for PSR-3 'Logger' (klimov-paul)
 - Enh #13706: 'Profiler' layer extracted (klimov-paul)
 - Enh #15410: Added serialization abstraction layer under `yii\serialize\*` namespace (klimov-paul)
+- Chg #10659: Method `runAction()` now is invoked for every module instance in the request handling chain, allowing custom implementation in particular module (klimov-paul)
+- Enh #15438: Added middleware support including PSR-15 'HTTP Server Request Handlers' compatibility (klimov-paul)


There are upcoming changes to be merged into PSR-15 spec soon.

php-fig/fig-standards#996

The changes are only clarifications, and will not touch the actual interfaces and their relations to one another.

samdark · 2018-01-07T21:21:04Z

framework/base/Controller.php

-            } else {
-                $runAction = false;
-                break;
+        $result = Yii::$app->getMiddlewareDispatcher()->dispatch($request, $this->middleware, function () use ($action, $params) {


Could be moved out of controller to upper level (router which is Application in our case).

samdark · 2018-01-07T21:22:42Z

framework/base/Module.php

+     * @see MiddlewareDispatcherInterface::dispatch()
+     * @since 2.1.0
+     */
+    public $middleware = [];


Same here. I think it would be great to try moving it to routing level.

Providing middleware at the routing level should be handled by separated changeset in the scope of #15363

samdark · 2018-01-07T21:23:01Z

framework/base/Module.php

     * @param string $route the route that specifies the action.
     * @param array $params the parameters to be passed to the action
     * @return mixed the result of the action.
     * @throws InvalidRouteException if the requested route cannot be resolved into an action successfully.
     */
-    public function runAction($route, $params = [])
+    public function runAction($request, $route, $params = [])


Could be type-hinted with Request.

Unlikely.
PSR middleware does not provide any garantee the original request instance to be preserved. It passes ServerRequestInterface instance over the tiers, but can modify it in process or even recreate at will.

public function process(ServerRequestInterface $request, RequestHandlerInterface $handler) { $newRequst = new ExternalRequest(); return $handler->handle($newRequst); }

Thus for the web application the correct typehinting will be ServerRequestInterface, while for the console application it will be yii\console\Request (or yii\base\Request).

yii\base\Module is suitable both for web and console application, thus such bounds can not be applied without introduction of the separated yii\web\Module and yii\console\Module. Such change will cause a lot of trouble. In particular: yii2-gii will have to implement 2 separated modules for the web and console interfaces, which then will require separated configuration.

It passes ServerRequestInterface instance over the tiers, but can modify it in process or even recreate at will.

Yes but what's returned is still ServerRequestInterface, isn't it?

Ah. I've got what you mean. Console. Alright then.

samdark · 2018-01-07T21:23:57Z

framework/composer.json

@@ -77,6 +77,9 @@
        "bower-asset/punycode": "1.3.*",
        "bower-asset/yii2-pjax": "~2.0.1"
    },
+    "suggest": {


I think we can require it safely. It's light dependency containing interface only.

There is no need for this package to be mandatory.
In case particular project does not use PSR middleware - this package is a simple garbage, which is never used. In case some project does use some PSR middleware - it is more likely to coming from another composer package, which will have 'http-server-middleware' among its dependencies for sure.

We're potentially converting action filters to middlewares, aren't we?

I insist on having it in require.

We're potentially converting action filters to middlewares, aren't we?

At the present state, we are not. If such changeset will appear, it will include composer.json changeset inside it.

I insist on having it in require.

Unless the official PSR-15 composer package appear this is risk prone. I can agree having mandatory dependency on psr/http-server-middleware but not on http-interop/http-server-middleware

OK. Makes sense. I hope PHP-FIG will finish PSR-15 soon and there will be official interface package available.

samdark · 2018-01-07T21:24:20Z

framework/console/Application.php

     * @throws Exception if the route is invalid
     */
-    public function runAction($route, $params = [])
+    public function runAction($request, $route, $params = [])


Could be type-hinted with Request.

Using yii\console\Request as a type restriction is a risk prone. Unless there is some yii\console\RequestInterface interface type-restriction introduction reduces flexibility here.

samdark · 2018-01-07T21:24:34Z

framework/console/Controller.php

     * @param string $id the ID of the action to be executed.
     * @param array $params the parameters (name-value pairs) to be passed to the action.
     * @return int the status of the action execution. 0 means normal, other values mean abnormal.
     * @throws InvalidRouteException if the requested action ID cannot be resolved into an action successfully.
     * @throws Exception if there are unknown options or missing arguments
     * @see createAction
     */
-    public function runAction($id, $params = [])
+    public function runAction($request, $id, $params = [])


Could be type-hinted with Request.

Same as #15459 (comment)

klimov-paul · 2018-01-08T10:39:16Z

I think that defining middleware stack should not be tied to modules/controllers but instead be done at router level.

Middleware can be inserted at any stage of the request processing. This PR provides 3 tiers:

controller level
particualr module level
whole application level

If I withdraw module-level middleware it will end up in complains of developers like @SamMousa, saying they are lacking flexibility at the module level.
Allowing middleware application for the module automatically allows it for the whole application.
Controller layer middleware can be useful as well, allowing passing request of articular module section to the external middleware.

As for defining middleware via URL rules - it should be applied as a separated changeset. It could be easily integrated at yii\web\Application::handleRequest().
This matter relates to #15363.

SamMousa · 2018-01-08T10:46:46Z

I see I'm getting a reputation :-p

samdark

All good except:

package dependency should be in require in composer.json.
Please take a look at: [PSR-15] Provide middleware dispatch examples php-fig/fig-standards#996

rob006 · 2018-01-27T18:16:42Z

psr/http-server-handler was released and http-interop/http-server-handler is deprecated.

https://packagist.org/packages/http-interop/http-server-handler
https://packagist.org/packages/psr/http-server-handler

Conflicts: composer.json framework/UPGRADE.md framework/composer.json tests/framework/console/ControllerTest.php

olleharstedt · 2018-07-30T10:03:12Z

Any progress on PSR-7 and PSR-15 support on Yii? Is it decided to be supported?

samdark · 2018-07-30T20:23:47Z

It is decided to be supported yes but no further progress was made yet.

samdark · 2019-03-12T11:43:46Z

Closing since it won't be applied to 2.x.

klimov-paul added 17 commits January 4, 2018 17:01

PSR request handlers created

6b1c1e8

Module::runAction() has been made traversable

06db64e

unit test fix

2f50b81

'middleware dispacther' layer extracted

197fe80

'middleware dispacther' layer applied

df1a37d

action result normalization moved to controller

b30e7b1

module/application middleware invocation fix

48a417b

MiddlewareDispatcher for web implemented

57e6993

CallbackMiddleware created

7c7851e

MiddlewareDispatcher draft implementation

042fa6d

unit tests fix

5dcb87a

console unit tests fix

24d0a87

fix DbMessageControllerTest

ec2c8d8

HelpCommand fix

818529c

update composer.lock

0dc362c

console actions tnu at unit tests fix

d4a9c30

unit tests fix

8cc4298

klimov-paul added severity:BC breaking Either breaks backwards compatibility or fixed previously made breakage type:enhancement labels Jan 5, 2018

klimov-paul added this to the 2.1.0 milestone Jan 5, 2018

klimov-paul assigned samdark Jan 5, 2018

klimov-paul mentioned this pull request Jan 5, 2018

Middleware #15294

Closed

klimov-paul added 4 commits January 5, 2018 14:00

base middleware created

0ab2019

'http-interop/http-server-middleware' composer package has been made …

1d571da

…optional

PHPDoc update

95bfacd

added CHANGELOG and UPGRADE

d4f2751

SilverFire reviewed Jan 6, 2018

View reviewed changes

SilverFire requested changes Jan 6, 2018

View reviewed changes

samdark reviewed Jan 7, 2018

View reviewed changes

samdark approved these changes Jan 9, 2018

View reviewed changes

klimov-paul added 4 commits February 7, 2018 17:32

'psr/http-server-middleware' package added

949184f

Merge branch '2.1' of github.com:yiisoft/yii2 into 15438-psr-middleware

1a59bee

Conflicts: composer.json framework/UPGRADE.md framework/composer.json tests/framework/console/ControllerTest.php

fix unit test

6e033cd

CHANGELOG.md fix

65257a9

samdark closed this Jun 22, 2018

rob006 changed the base branch from 2.1 to 3.0 July 7, 2018 15:48

rob006 reopened this Jul 7, 2018

klimov-paul mentioned this pull request Jan 16, 2019

Support PSR-15 HTTP Middleware yiisoft/yii-web#11

Closed

samdark closed this Mar 12, 2019

Fix #15438 PSR middleware #15459

Fix #15438 PSR middleware #15459

Conversation

klimov-paul commented Jan 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SilverFire commented Jan 6, 2018

SilverFire left a comment

Choose a reason for hiding this comment

klimov-paul commented Jan 6, 2018

samdark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klimov-paul Jan 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klimov-paul commented Jan 8, 2018

SamMousa commented Jan 8, 2018

samdark left a comment

Choose a reason for hiding this comment

rob006 commented Jan 27, 2018

olleharstedt commented Jul 30, 2018

samdark commented Jul 30, 2018

samdark commented Mar 12, 2019

klimov-paul Jan 8, 2018 •

edited

Loading