[RFC][HttpFoundation] Introduce RequestMatcher as a namespace #20274
Conversation
|
I don't see which problems it solves. Request matchers should stay quite simple. |
|
Maybe it's a bridge too far, but personally i would like to be able to write a request matcher as follow; class MyMatcher extends ChainMatcher {
public function __construct() {
parent::__construct([
new MethodMatcher(['GET', 'POST']),
new PathMatchher('/some-path'),
]);
}
}Or be able to just use a path matcher, if path matching is the only case (opposed to the full generic request matcher we have now). From my experience things like access_control uses only path matching 9/10 times. Imo. that should built a chain matcher based on given configs.., ie. $matchers = [];
if (isset($config['path'])
$matchers[] = new PathMatcher($config['path']);
}
// etc.
$matcher = 1 === count($matchers) ? reset($matchers) : new ChainMatcher($matchers);It makes things simple, clean and reuseable on a very low level. However, my true interest goes to #20272, which made me think about this approach. The current (generic) request matcher would be just a built-in chain request matcher. Please close, if you dont think this is viable or over-engineered. |
| { | ||
| $matchers = array(); | ||
| if (null !== $path) { | ||
| $matchers[] = new PathRequestMatcher($path, true); |
There was a problem hiding this comment.
In order to make it even more reusable it would be nice to be able to inject custom matchers:
/**
* @param RequestMatcherInferace|string|null $path
* @param RequestMatcherInferace|string|null $host
* @param RequestMatcherInferace|string[] $methods
* @param RequestMatcherInferace|string[] $ips
* @param RequestMatcherInferace|array $attributes
* @param RequestMatcherInferace|string[] $schemes
*/
public function __construct($path = null, $host = null, array $methods = array(), array $ips, array(), array $attributes = array(), array $schemes = array())
{
$matchers = array();
if (null !== $path) {
$matchers[] = $path instanceof RequestMatcherInterface ? $path : new PathRequestMatcher($path, true);
}
// ...There was a problem hiding this comment.
Not sure.. to me this class just factorizes a chain matcher (from scalars). If you have ready-to-use matchers, just chain them yourself 😕
And eventually the entire class may not be needed, if we built a chain in the security extension from config.
Same for the current ExpressionRequestMatcher, which extends the (current) generic request matcher and integrates the EL component additionally. Now we have 2 generic request matchers... 😕
With this PR i propse a standalone expression request matcher, which you can combine with other matchers (using a chain).
| */ | ||
| public function __construct(array $matchers) | ||
| { | ||
| $this->matchers = $matchers; |
There was a problem hiding this comment.
You should loop through the matchers and maybe use a public add or addMatcher() which throws an invalid argument exception when it does not implement the RequestMatcherInterface.
There was a problem hiding this comment.
👍 not sure about a add method though, making the class mutable. I tend to like having immutable matchers by default.
|
👎: a request is a single semantic unit of path+host+method+ips+scheme+etc. Having a single matcher just matches this unit. |
|
Closing in favor of #21029, only this wont change much. |
Idea is to have many simple request matchers instead of several complex ones (allowing to built complex ones from simple ones) and make them immutable.