Update middleware docs. #5096
Update middleware docs. #5096
Conversation
markstory
commented
Jul 17, 2017
markstory
commented
- Add new group features.
- Better explain how scoped middleware works.
- Add onion images. Refs Add Middleware Onion to the docs #5087
* Add new group features. * Better explain how scoped middleware works. * Add onion images. Refs #5087
|
Something that confused me with this image during your talk is that it looks like the response goes through the middleware stack as well. |
| Scoped Middleware | ||
| ----------------- | ||
|
|
||
| Middleware can now be conditionally applied to only routes in specific URL |
There was a problem hiding this comment.
"to only routes in specific URL scopes."
The "only" seems to be misplaced. Not an English native speaker, though.
"to routes in specific URL scopes, only."
May be? Sounds more correct to me, at least.
en/controllers/middleware.rst
Outdated
| class Application extends BaseApplication | ||
| { | ||
| public function middleware($middlewareStack) | ||
| public function middleware($middleware) |
There was a problem hiding this comment.
What's the reasoning behind this renaming? I'd prefer with the "Stack" suffix.
There was a problem hiding this comment.
https://github.com/cakephp/app/blob/master/src/Application.php#L37
We currently name the parameter $middlewareQueue in the app skeleton.
There was a problem hiding this comment.
Thought so. Is there a reason not to change this in the app skeleton?
There was a problem hiding this comment.
I prefer the it without or with Stack... Queue is in the "people will miss-type it" ballpark and we should IMHO change the app skeleton if possible.
Edit: But then there is \Cake\Http\MiddlewareQueue which $middleware references a copy of...
There was a problem hiding this comment.
It was already changed: cakephp/app#507 so why changing it again?
There was a problem hiding this comment.
Oh, thanks for pointing that out. But then it should be named middlewareQueue, as in https://github.com/cakephp/app/blob/6d9a9909fea0f06fe305d6095c1b33d3cac65e80/src/Application.php#L39. Otherwise we need to update the docs later again.
| $routes->registerMiddleware('cookie', new EncryptedCookieMiddleware()); | ||
| $routes->registerMiddleware('auth', new AuthenticationMiddleware()); | ||
| $routes->registerMiddleware('csrf', new CsrfProtectionMiddleware()); | ||
| $routes->middlewareGroup('web', ['cookie', 'auth', 'csrf']); |
There was a problem hiding this comment.
"web" seems a very general. How about "authenticated_site" or amything like that?
There was a problem hiding this comment.
I like web, because it is not about auth here but about the context of middleware application.
On the web context you pass request/response through cookie, auth, csrf
@jeremyharris The response does go through all the layers on the way out as well. |
|
@markstory oh! Learned something new today! 🎉 Edit: d'oh, of course it does! All middleware takes the response and request in |
| 'Server.buildMiddleware', | ||
| function ($event, $middlewareStack) { | ||
| $middlewareStack->add(new ContactPluginMiddleware()); | ||
| function ($event, $middlewareQueueStack) { |
There was a problem hiding this comment.
... this gave me some laughs... :)
So now it is sometimes middlewareQueue and sometimes middlewareQueueStack ;)?
Personally I'd rename the core class to MiddlewareStack and alias it from middlewareQueue to be removed in 4.0. Stack is easier to type and speak.
There was a problem hiding this comment.
That is a mistake. I will get that fixed.
