[RFC][Routing] made RequestContext optional in generator and matcher #5895

Tobion · 2012-11-02T11:53:09Z

tests pass: yes
bc break: yes but only tiny and none for the symfony implementations.
todo:

many tests
Router should also implement RequestMatcherInterface - needs PhpMatcherDumper adjustments
TraceableUrlMatcher would also need to handle a Request
RedirectableUrlMatcher would also need to handle a Request
ApacheUrlMatcher would also need to handle a Request

After discussion with @Crell, we agreed that the RequestContext is a burden in reusing the routing component. That's because it only duplicates information of the Request and is not really useful without it anyway. Since we probably cannot remove it fully as it would be a big BC break, my idea is to make it optional. So the symfony generator and matcher both work with a RequestContext and a Request. I removed the RequestContextAwareInterface from the interfaces so a custom router does not need a RequestContext anymore. The default symfony generator/matcher still implements RequestContextAwareInterface but can also work with a Request. So UrlMatcher::match(Request) is now also possible. And the generator can use UrlGenerator::setRequest() instead of setContext().

I think it makes the routing more useable in symfony and more reusable for other libraries.
And it will probably make it easier to integrate content negotiation as matching a request directly with all it's metadata (like Accept header) is now possible.

One more point: symfony allowed to use matching a Request with #4363. But it's not integrated in symfony as symfonys router does not use it at all. And the important interfaces like the RouterInterface required the RequestContext anyway. So there was no way to use Request matching without RequestContext. This PR replaces the other extension point.

The only bc break is in terms of interfaces. People should be aware that RequestContextAwareInterface is not part of UrlGeneratorInterface/UrlMatcherInterface/RouterInterface anymore. And the RequestMatcherInterface was changed.

Tobion · 2012-11-02T12:40:35Z

src/Symfony/Component/Routing/Generator/UrlGenerator.php

    {
        $this->routes = $routes;
-        $this->context = $context;
+        if ($context instanceof Request) {


Does it trigger autoloading? If so it would make HttpFoundation a required dependency. But the condition can be switched, so is not required.

instanceof does not trigger autoloading.

On Fri, Nov 2, 2012 at 1:40 PM, Tobias Schultze notifications@github.comwrote:

In src/Symfony/Component/Routing/Generator/UrlGenerator.php:

{ $this->routes = $routes;

$this->context = $context;

if ($context instanceof Request) {

Does it trigger autoloading? If so it would make HttpFoundation a required
dependency. But the condition can be switched, so it isn't.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/5895/files#r2015580.

ok thanks for the info

stof · 2012-11-02T13:47:24Z

you probably need to check the RouterListener to add some instanceof RequestContextAwareInterface checks (as well as in any other places using the router). Modifying a method by removing methods in it is a BC break.

Tobion · 2012-11-02T15:59:18Z

src/Symfony/Component/Routing/Matcher/RequestMatcherInterface.php

-     * @param Request $request The request to match
+     * @see UrlMatcherInterface
+     *
+     * @param Request|string $request A Request or a string with the path component of a URL (raw format, i.e. not urldecoded).


Maybe it's better to say, that RequestMatcherInterface only need to support Request and not necessarily a path string, so they don't need to deal with that case. So UrlMatcherInterface need only support string and RequestMatcherInterface need only support Request.

@Tobion you cannot do it. This PR makes it extend UrlMatcherInterface so it has to respect the contract of the UrlMatcherInterface otherwise it is simply broken.

yeah thought about that too. and it also makes sense that RequestMatcherInterface extends UrlMatcherInterface as the path is a component of a request. so it shouldnt be a problem.

A request-centric matcher can easily turn a path string into a request by creating a new Request object, which would then behave identically for the rest of the method. I think it's fine to require supporting both.

fabpot · 2013-01-02T09:45:22Z

src/Symfony/Component/Routing/Generator/UrlGenerator.php

+        if (null !== $this->context) {
+            $mergedParams = array_replace($defaults, $this->context->getParameters(), $parameters);
+        } else {
+            $mergedParams = array_replace($defaults, $parameters);


That makes me think that when not using a RequestContext, there is no way to define global default parameters.

That's true. It's mostly used for the _locale. For this we can use Request::getLocale() as default parameter for a _locale placeholder. This is then equivalent to the HttpKernel/EventListener/LocaleListener but for a Request object.

fabpot · 2013-01-02T09:48:21Z

Overall, this PR looks ok to me. @Tobion I see that you have some todo items in the PR description, can you fix thembefore I merge?

fabpot · 2013-01-09T08:25:21Z

@Tobion Anything left before I merge this PR?

Tobion · 2013-01-09T09:08:36Z

@fabpot Yes, I did not have time to work on the todos yet. Hope I will make it this evening.
Also I'd be glad if #3958 gets merged first.

fabpot · 2013-01-09T09:41:46Z

#3958 has been merged now.

fabpot · 2013-01-16T08:18:09Z

Any chance you can finish this soon. Changes like this one need to be merged before the end of this week.

So the usage of RequestContext is now optional everywhere because people can use a Request directly instead. For this to achieve, the generator and matcher interface is now independent from RequestContextAwareInterface and the RequestMatcherInterface now extends the UrlMatcherInterface.

This is because if no RequestContext is set before calling get, there is no context that could be returned.

it also fixes the type hints that didnt make much sense

Unfortunately this is not allowed in PHP5.3 which we support, so we cannot override the phpdoc for the method either

Tobion · 2013-01-24T13:05:19Z

Hm this is quite some work because I would also need to make TraceableUrlMatcher, RedirectableUrlMatcher and ApacheUrlMatcher work with a Request object. Also many test would be necessary theoretically to cover all.

Also I'm not quite happy with it in general. Having UrlGenerator implement both setContext(RequestContext $context) and well as public function setRequest(Request $request) is quite strange. Because users would wonder what to use and it means we need to keep both a request property and a context property so we can return the appropriate one from their getters. Also having UrlGeneratorInterface and UrlMatcherInterface extend RequestContextInterface made sense because it's where the metadata is set that the generator and matcher need. So splitting it, is also quite strange. Because an UrlMatcher without a context doen't make sense.
Also having the generator implement setRequest is not really reusable because a generator is usually typehinted against UrlGeneratorInterface. But it does not have a setRequest. So basically others cannot use setRequest without typehinting against the implementation instead of the interface.

So instead of the current approach I'd like to suggest a different one:
Revert the splitting of Generator/Matcher and the RequestContextInterface so removing this BC break for it. But instead change RequestContextAwareInterface to public function setContext($context); where $context can be either a RequestContext or a Request. This would be a bigger BC break because it changes the signature (removing the typhint), but I think it's much cleaner, removes the problems above and is the better solution in the long run.

@fabpot what do you think?

… Request correctly

fabpot · 2013-01-24T15:14:21Z

Theoretically, I do like the idea of having the context as a RequestContent object or a Request object. Now, let's see of the code looks like with this change.

fabpot · 2013-03-26T08:43:55Z

@Tobion: any update on this PR? Can we close it and open a ticket for the suggestion in your last comment instead?

igorw · 2013-03-26T08:47:43Z

src/Symfony/Component/Routing/RequestContext.php

@@ -56,7 +56,7 @@ public function __construct($baseUrl = '', $method = 'GET', $host = 'localhost',
        $this->scheme = strtolower($scheme);
        $this->httpPort = $httpPort;
        $this->httpsPort = $httpsPort;
-        $this->pathInfo = $path;
+        $this->pathInfo = $pathinfo;


Casing should be consistent here.

Tobion · 2013-03-26T08:50:28Z

I'd leave it open because I would also work on the new implement in this PR when I find some time. And the PR motivation description remains the same.

fabpot · 2013-04-20T20:26:38Z

Is it worth working on this at this point? Especially as we won't break BC anymore.

fabpot · 2013-08-09T06:35:49Z

Closing this PR as the code is not finished. I've opened an issue to keep track of this possible change (see 8C28 #8705).

Tobion reviewed Nov 2, 2012
View reviewed changes

Tobion mentioned this pull request Dec 30, 2012

added MatcherInterface as a marker for all Routing matcher classes #6509

Closed

fabpot reviewed Jan 2, 2013
View reviewed changes

Tobion added 6 commits January 17, 2013 20:52

Router implements RequestMatcherInterface (WIP)

5aec4d9

[HttpKernel] RequestContextAwareInterface::getContext could return null

4bc716c

This is because if no RequestContext is set before calling get, there is no context that could be returned.

[WebProfiler] use the new request matching possibility

93a1fbf

it also fixes the type hints that didnt make much sense

[Routing] rename variable for consistency

6b808ad

[Routing] remove redeclaration of interface method

45474e7

Unfortunately this is not allowed in PHP5.3 which we support, so we cannot override the phpdoc for the method either

Tobion added 4 commits January 24, 2013 15:53

fix uses

7a80b48

[FramworkBundle] made typehints in RouterDebugCommand more sensible

af3cba4

[Routing] fix ApacheUrlMatcher and RedirectableUrlMatcher to accept a…

6c22f96

… Request correctly

[Routing] fix phpdoc of Router

49fbd6f

igorw reviewed Mar 26, 2013
View reviewed changes

fabpot closed this Aug 9, 2013

fabpot mentioned this pull request Feb 25, 2019

make RequestContext optional in generator and matcher #8705

Closed

Tobion deleted the requestcontext branch September 10, 2019 20:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC][Routing] made RequestContext optional in generator and matcher #5895

[RFC][Routing] made RequestContext optional in generator and matcher #5895

[RFC][Routing] made RequestContext optional in generator and matcher #5895

[RFC][Routing] made RequestContext optional in generator and matcher #5895

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment