3.0 middleware events #3495
Merged
3.0 middleware events #3495
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| * example, if you only wanted a filter applied to blog requests you could do: | ||
| * | ||
| * {{{ | ||
| * $ware = new BlogMiddleware(['for' => '/blog']); |
There was a problem hiding this comment.
is routes connected yet? e.g. can 'for' be an ['controller' => 'something'] ?
There was a problem hiding this comment.
Wasn't the suffix Filter instead of Middleware?
There was a problem hiding this comment.
@jippi that can be achieved with when
There was a problem hiding this comment.
Bah, I will fix this when I complete the doc block changes.
|
looking awesome! |
The renames to middleware still need to happen, but my gut feeling is that there isn't anything seriously broken with how filters work right now beyond the configration bits.
Default to the property (which is now protected) but allow users of a filter to redefine the priority.
Clean up the remaining dispatcher tests and try to cover the bulk of the cases.
Like many other places in CakePHP we should allow string classnames for filters, and let the framework handle construction for simple cases.
The additionalParams parameter isn't very useful as it simply gets merged into the request later on. We can easily do that earlier in the process now. Fix incorrectlty dispatched array based requestAction() calls. They were going back through Router::parse() when they did not need to.
Add in knowledge of dispatcher filters. Also workaround Routing being applied in a filter vs. a Dispatcher method so that the rest of ControllerTestCase can continue to work.
Use a Filter suffix for dispatcher filters. I think this will make them easier to recognize at a glance and mirrors other classname conventions in CakePHP.
The feature is no longer called middleware, instead continue using dispatcher filters. We can't really call this feature middleware as it acts very differently from middleware in other frameworks.
When using the string form, it is handy to be able to control the constructor arguments as well.
Add some detail around how priorities are handled.
|
@lorenzo I've added the ControllerFactoryFilter. |
| public function handle(Event $event) { | ||
| $name = $event->name(); | ||
| list($unused, $method) = explode('.', $name); | ||
| if (empty($this->_config['for']) && empty($this->_config['when'])) { |
There was a problem hiding this comment.
@markstory Shouldn't this condition be part of matches? Seems it can be an earlier return as true.
There was a problem hiding this comment.
It could, I was prematurely optimizing away the method call to matches().
Having a separate filter that handles controller resolution provides a few nice things: * It makes it easy for applications to extend/replace the controller conventions. * It makes dispatcher super simple internally. * It makes CakePHP less magic. No unit tests were added as the existing Dispatcher test cases cover all the functionality in the ControllerFactoryFilter. Fix up the ControllerTestCase as it can no longer override the Dispatcher internals to do its work. Instead a special filter is applied that will replace the controller with a mock if necessary.
Spooky side-effects at a distance on object construction is a no-no.
This allows cached/asset requests to not hit the router.
|
Awesome work @markstory ! |
This was referenced May 16, 2014
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This set of changes updates dispatcher filters. I explored the 'more robust' middleware implementation described in #2544 and decided it was a bad idea for a few reasons:
So instead of throwing out the existing code, I've done a few incremental changes, mostly around making it easy for filters to only run on specific URL's or when specific conditions are met. I've also introduced what I think is a simpler/better way to manage filters with the
DispatcherFactory.I have a few todo's left, but I wanted to get some feedback on this before finishing everything.
TODO