RequestFilter adjustments #3130
Conversation
| $filterResponse = Injector::inst()->get('RequestProcessor')->preRequest($req, $session, $model); | ||
| if ($filterResponse && $filterResponse!==true) { | ||
| if (!$filterResponse instanceof SS_HTTPResponse) { | ||
| throw new SS_HTTPResponse_Exception('RequestFilters have to return null, true or SS_HTTPResponse.'); |
There was a problem hiding this comment.
You should use user_error here. A legitimate programming error shouldn't raise a SS_HTTPResponse_Exception, only user-initiated errors.
You also need to handle the 'false' case, or are we dropping that from the API?
Allow pre-filters to override the framework response. Previously, only throwing an Exception was possible. This change will allow us to do pre-request caching, rate-limiting, 304 Not Modified responses etc. Fix the post-filter order execution to be more intuitive: highest priority goes last. Adds ability to add filters during the run-time. Refactor to allow returning SS_HTTPResponses from post filter.
|
Needs some tests. |
|
And needs fixing Director::test :-) |
| // No-op | ||
| } | ||
|
|
||
| public function postRequest(\SS_HTTPRequest $req, \SS_HTTPResponse &$res, \Session $session, \DataModel $model) { |
There was a problem hiding this comment.
Why not keep the parameters in the same order? (request, response, session, model).
Also, I see value in having well defined names (request over req, earlyResponse over earlyRes). What's the argument behind shortening these?
There was a problem hiding this comment.
I wish PHP had named parameters like ruby. :)
There was a problem hiding this comment.
Hmm did you misread that? The order is the same everywhere - request, response, session, model. Where is it different?
Shortened res and req for readability - otherwise the line needs to be broken up and the whole function header starts looking huge.
There was a problem hiding this comment.
What I mean is, in 3.1 the order is "request response model" and there is no session. What I'm suggesting is that you add session to the end, rather than move model to the 4th parameter.
There was a problem hiding this comment.
On the parameter ordering, if I do as you are suggesting, then the postRequest and preRequest will have mismatched order - one will have session last, second one will have session last-but-one. See the proposed interface:
public function preRequest(SS_HTTPRequest $req, Session $session, DataModel $model);
public function postShorted(SS_HTTPRequest $req, SS_HTTPResponse &$earlyRes, Session $session, DataModel $model);
public function postRequest(SS_HTTPRequest $req, SS_HTTPResponse &$res, Session $session, DataModel $model);There was a problem hiding this comment.
Ok, you have my permission to break everything. It's not like 3.1 -> 3.2 is going to be a smooth transition in any case.
There was a problem hiding this comment.
Most important consideration when breaking API between minor releases is: will the change be immediately obvious, or will everything look like it's still working, but be introducing subtile errors into your logic.
In this case, because of the typing of the arguments, trying to use the old API will trigger an error, so it's probably OK.
There was a problem hiding this comment.
Thanks @hafriedlander - that sounds like a reasonable approach.
|
Just a general question, do you think it's worth allowing for the order of request filters to be specified, similar to how routes are? It would make sense if some filters simply setup the state (versioned) and others deal with generating responses. |
|
Hmm the filters are specified as an array, just as the routes are. Ordering is controlled through the config system's before and afters, so if we have a problem with this not being powerful enough, we should probably fix the config system instead of hacking here :-) m |
|
The problem is that before and after are only reliable ways to ensure order of evaluation, not order of merge. :) Still, if it's hard, we can just not do it for now. |
|
Isn't the order of evaluation equate to the order of the final array? If that's not working - you could always name your module zzz_damian :-) |
|
Maybe just document that it's not reasonable to rely on execution order just yet. |
|
Changed the names from req to request and res to response. I will have a look how to do the ordering, because it seems important here. |
|
@hamish, @tractorcow, @halkyon while I have a captive audience here: do we want to have a framework support for filters before/after controllers and controller actions? Note that it is possible to achieve the "after" filters using the existing global RequestFilters and existing extension points (see silverstripe-controllerpolicy), but it's a little hackish. To apply these "before", one would need to roll their own solution. Does it make sense to look into institutionalising these? |
|
I like the below syntax as an option. I tested this with injector and it seems to work, but the request processor would have to do the sorting in setFilters. ---
Name: requestprocessors
---
Injector:
RequestProcessor:
properties:
filters:
# Default to 0 order
- '%$VersionedRequestFilter'
# Lower order means first, higher means later
- filter: '%$MyLowPriorityFilter'
order: 10 |
|
Ideally this would all be done with events. But events don't exist yet, so we've brought in the request pre/post filter. So my feeling is: given we haven't seen a clear and present need for them, leave them out for now, and we'll do events in 3.2. However it's not an argument made from a position of strength, since I've been wanting to do events for a while, and so far.... |
|
As a note, using 'before: #other' and 'after: #other' have the opposite result to what's expected. ---
Name: myconf
---
Page:
matrix:
- Two
- Three
---
Name: myfirstconf
Before: '#myconf'
---
Page:
matrix:
- One
---
Name: mylastconf
After: '#myconf'
---
Page:
matrix:
- Four Produces: |
|
We have a clear need for them. We wanted action-based throttling and caching :-) @hafriedlander, do you have any kind of write-up on the events, or it's all in your head? :-) |
|
Mostly in my head. A little bit in Damien's too. There's a module someone's done that was close, but needed work to examine how performant it was (need to be able to look up event receivers without loading all classes into memory) |
|
I'll back off then, since I see you've got it sorted. This fix above was meant to be for 3.2, so sounds like it can be closed. Oh well, good learning experience :-) |
|
As for ordering, I like using the config system. |
|
Then everyone is happy, and people ordering and prioritising things actually get the code to do what it says it does. :) Instead of doing what it says it does, but using the wrong words. |
|
The ordering through the configuration system is confusing, even though I know what each one does and why, I regularly have to check myself when working out which to use. I also do the same with implode/explode though, so it may just be my brain. Renaming to |
|
Or 'over' and 'under'? |
| */ | ||
| public function postShorted( | ||
| SS_HTTPRequest $request, | ||
| SS_HTTPResponse &$earlyResponse, |
There was a problem hiding this comment.
Shouldn't be a reference. Likewise throughout.
|
Unless someone can actually commit to getting an events system in for 3.2, then I don't feel like it should be used as a reason to not add controller filters. |
|
@hafriedlander Will it be possible to add the events system in https://github.com/heyday/silverstripe-cacheinclude/blob/master/src/RequestCache.php |
|
@camspiers yeah, can be done in a backwards compatible way (just have an included-as-standard listener on the event that then calls all the RequestFilters) |
|
Needs more work, and a decision as to whether we do it or not. Closing for now. |
Make the API more useful by allowing to override the response in pre-request filter so we can build rate-limiting, caching, 304 responses and so on.
Just return non-true from the pre-request handler, and the framework will try to parse it as a response value. Return SS_HTTPResponse to get that delivered to the user.
Needs some more testing - I've only checked against https://github.com/mateusz/controllerpolicy