Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

Added strategies to the BodyParamsMiddleware #5

Merged
merged 5 commits into from
Dec 22, 2015

Conversation

weierophinney
Copy link
Member

This patch builds on the one in #3, moving the act of parsing a Content-Type as well as parsing the request body to strategies, which implement Zend\Expressive\BodyParams\StrategyInterface. These define two methods:

  • match($contentType), which should accept a Content-Type header value, and return a boolean value indicating whether or not the strategy matches.
  • parse(ServerRequestInterface $request), which should accept the request instance, parse its raw body, and return a new request instance with the results of parsing injected as the parsed body.

The form and json matching/parsing were rewritten as strategies, and added as default strategies to the middleware. Custom strategies can be written and then attached using addStrategy().

michaelmoussa and others added 5 commits December 14, 2015 23:50
BodyParams middleware stolen w/permission from mwop.net
This patch builds on the one in zendframework#3, moving the act of parsing a Content-Type as
well as parsing the request body to *strategies*, which implement
`Zend\Expressive\BodyParams\StrategyInterface`. These define two methods:

- `match($contentType)`, which should accept a Content-Type header value, and
  return a boolean value indicating whether or not the strategy matches.
- `parse(ServerRequestInterface $request)`, which should accept the request
  instance, parse its raw body, and return a new request instance with the
  results of parsing injected as the parsed body.

The form and json matching/parsing were rewritten as strategies, and added as
default strategies to the middleware. Custom strategies can be written and then
attached using `addStrategy()`.
@weierophinney weierophinney added this to the 1.3.0 milestone Dec 22, 2015
@weierophinney
Copy link
Member Author

@michaelmoussa This is what I meant by strategies. 😄

@michaelmoussa
Copy link
Contributor

@weierophinney This makes a lot of sense and really eases extensibility! I wonder if there might be other common types of middleware in which strategies like this would be useful?

I can't think of any non-trivial examples off the top of my head, and I really like how it's done in this PR, but I can't help but wonder if there's a sensible, meaningful way to have a more generic StrategyInterface and some corresponding more generic interface that a middleware could implement to indicate that it supports the use of strategies as a way of encouraging consistency among other similar helper middlewares.

Maybe something to think about for a later date, but what's here now looks good and definitely satisfies my original need that led me to submit the PR in the first place.

👍

@weierophinney weierophinney merged commit 368e8ed into zendframework:develop Dec 22, 2015
weierophinney added a commit that referenced this pull request Dec 22, 2015
weierophinney added a commit that referenced this pull request Dec 22, 2015
@weierophinney weierophinney deleted the feature/3 branch December 22, 2015 20:32
@snapshotpl
Copy link
Contributor

Nice one. Can someone explain me what FormUrlEncodedStrategy is doing?

@weierophinney
Copy link
Member Author

@snapshotpl It's in there to match as early as possible, and return the request verbatim.

The reason: we don't want to loop through all strategies if we don't need to, and that's likely the most common match. The typical PSR-7 implementation will have populated the body parameters from $_POST in that case, so there's nothing left to do. By having it as the first in the list of strategies, we get a small performance optimization, as there will be only the one match performed (vs. attempting to match all strategies).

@snapshotpl
Copy link
Contributor

@weierophinney thanks for clear explanation

@weierophinney
Copy link
Member Author

I can't help but wonder if there's a sensible, meaningful way to have a more generic StrategyInterface and some corresponding more generic interface that a middleware could implement to indicate that it supports the use of strategies as a way of encouraging consistency among other similar helper middlewares.

@michaelmoussa — while there's a part of me that likes this idea, there's another part that doesn't. Strategy patterns are usually highly specific to a given domain. As an example, the approach in this patch requires:

  • matching the incoming content-type header, and
  • parsing the request body and returning a new request with the parsed body parameters, and
  • ensuring that the above two occur in exactly that sequence.

As such, a generic approach likely wouldn't work; we'd still need to typehint or perform type checks internally on the more specific interface that provides those capabilities.

What we can do is start looking at new middleware that crops up that could benefit from a similar approach, and make sure that the general approach of attaching strategies is the same. In other words, ensure that these each use addStrategy() and clearStrategies() in their implementations, to ensure consistency of use across different middlewares using the approach.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants