[Routing] RequestMatcherInterface doesn't need context #4582

Tobion · 2012-06-14T21:33:34Z

Matchers that implement RequestMatcherInterface should match a Request, thus they don't need the request context.

travisbot · 2012-06-14T21:39:48Z

This pull request fails (merged f5ff1fe0 into 7c91ee5).

stof · 2012-06-14T21:50:36Z

src/Symfony/Component/HttpKernel/EventListener/RouterListener.php

            if ($this->matcher instanceof RequestMatcherInterface) {
                $parameters = $this->matcher->matchRequest($request);
            } else {
+                if (HttpKernelInterface::MASTER_REQUEST === $event->getRequestType()) {
+                    $this->matcher->getContext()->fromRequest($request);


this means that the RequestContext is not initialized anymore whereas the generator uses it too, thus breaking things.

but it's the context of the matcher. if the generator needs an updated context it should be updated on the generator.
otherwise this listener simply assumes that the generator and matcher use the same context instance which is flawed imo.

@Tobion the matcher and the generator are meant to share the same context, otherwise it becomes a pain to support parameters in the context (for instance when implementing host-based routing, where different contexts would be a mess as soon as they become out of sync)

I see no other solution than injecting the generator too in the RouterListener. As this PR removes the RequestContextAwareInterface from the RequestMatcher (which makes total sense, don't you think?), it means when using a RequestMatcher it cannot initialize the context for the generator.
So we need to inject the generator, which would also remove the confusing and interdependent implementation.

@Tobion but in the context of the full-stack framework, what is injected is the router (which implements both the MatcherInterface and the GeneratorInterface and lazy-loads the actual matcher and generators from the cache). Injecting the same object twice seems really weird

I think the default instantiation didn't make much sense anyway because the default values (like localhost) will not be useful in 99% of the cases.

So where should I inject the context and where retrieve it from DIC? I'm not sure about the implementation suggested by you guys.

@Tobion instead of injecting the UrlGenerator (as suggested previously) to get the RequestContext from it, simply inject the RequestContext itself (in the constructor like other deps)

Ah ok, thought there was more to it.

Done, ready for review.

schmittjoh · 2012-06-15T13:32:59Z

I think it makes sense to remove the RequestContext from the RequestMatcher.

schmittjoh · 2012-06-15T13:34:17Z

src/Symfony/Component/HttpKernel/EventListener/RouterListener.php

        try {
-            // matching requests is more powerful than matching URLs only, so try that first
+            // matching a request is more straightforward than matching a URL path + context, so try that first


Why changing this comment? I think the previous one was better.

Because the comment was wrong. It matches path + context and not "only URLs", which is almost identical to the request.

almost identical ? Look at what is available in the Request. There is far more stuff in it.

Ok, but what I don't like is the word "straightforward" here, I think "powerful" is way better

ok i can revert the term

…rface

travisbot · 2012-06-15T15:54:28Z

This pull request passes (merged 7464dcd into f881d28).

Tobion · 2012-06-26T12:32:06Z

Anything missing?

Commits ------- 7464dcd added phpdoc c413e7b [Routing] remove RequestContextAwareInterface from RequestMatcherInterface 921be34 [Routing] fix phpdoc Discussion ---------- [Routing] RequestMatcherInterface doesn't need context Matchers that implement RequestMatcherInterface should match a Request, thus they don't need the request context. --------------------------------------------------------------------------- by travisbot at 2012-06-14T21:39:48Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1624496) (merged f5ff1fe0 into 7c91ee5). --------------------------------------------------------------------------- by schmittjoh at 2012-06-15T13:32:59Z I think it makes sense to remove the RequestContext from the RequestMatcher. --------------------------------------------------------------------------- by travisbot at 2012-06-15T15:54:28Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1628931) (merged 7464dcd into f881d28). --------------------------------------------------------------------------- by Tobion at 2012-06-26T12:32:06Z Anything missing?

[Routing] fix phpdoc

921be34

stof reviewed Jun 14, 2012
View reviewed changes

schmittjoh reviewed Jun 15, 2012
View reviewed changes

Tobion added 2 commits June 15, 2012 17:27

[Routing] remove RequestContextAwareInterface from RequestMatcherInte…

c413e7b

…rface

added phpdoc

7464dcd

fabpot merged commit 7464dcd into symfony:master Jul 1, 2012

Uh oh!

[Routing] RequestMatcherInterface doesn't need context #4582

[Routing] RequestMatcherInterface doesn't need context #4582

Uh oh!

Conversation

Tobion commented Jun 14, 2012

Uh oh!

travisbot commented Jun 14, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmittjoh commented Jun 15, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

travisbot commented Jun 15, 2012

Uh oh!

Tobion commented Jun 26, 2012

Uh oh!

Uh oh!