DynamicRouter can't be used exactly like ChainedRouter

[dawehner]

Given the architecture I'd expect that you could switch off ChainedRouter without having any problem,
but it turns out, you require it for ->match() calls

[dbu]

applications should only use what the RouterInterface or the UrlMatcherInterface / RequestMatcherInterface provides.

what exactly is the problem? either its a bug of DynamicRouter or ChainedRouter is cleaning up something wobbly that you are doing, so more hiding a problem you already have.

[dawehner]

So if you use DynamicRouter directly but your matcher is NestedMatcher it just doesn't work, its a bit sad that DynamicRouter does not also provide the wrapping like ChainedRouter does

[dbu]

do you use match or matchRequest? the preferred way to use any of the routers is to use matchRequest, to prevent information loss. anything but the URL has to be made up out of thin air with match()

[dawehner]

HI.

Well, for normal routing we use ::matchRequest() which works perfect. For some internal validation of user entered path we used to use ::match() but I think its fine to just replace that single call

[dawehner]

See https://www.drupal.org/files/issues/2576809-11.patch for the adaptions which needed to be made

[dbu]

if i read that diff correctly, the only change related to the different behaviour is this:

-      return $router->match($path);
+      $subrequest = Request::create($path);
+      return $router->matchRequest($subrequest);

this seems not too heavy to me.

the rest is having to do with assumptions and the different interfaces that may or may not be implemented when you want to be generic.

+    if ($this->chainRouter instanceof RequestContextAwareInterface) {
+      $this->chainRouter->getContext();
+    }
+    throw new \RuntimeException();

this is missing a return i think.

i don't know the context but i wonder why you do not require a RouterInterface to be injected if you want to decorate a router.

+    if ($this->chainRouter instanceof RouterInterface) {
+      return $this->chainRouter->generate($name, $parameters, $referenceType);
+    }
+    throw new \RuntimeException();

[dbu]

btw, also not #147 and #152 where people want to change how the ChainRouter match vs matchRequest behaves. and even more related things, there is the idea to only have a matchRequest method in symfony 3: symfony/symfony#9781

[dbu]

@dawehner btw, #147 is about Request::create() doing unexpected things on //foo (since some symfony bugfix, this is no longer considered a path but a domain without protocol).

[lsmith77]

instead of throwing an exception if we do not have an UrlMatcherInterface, we duplicate the same logic from the ChainRouter::doMatch() method .. does that work for you?

[dawehner]

Yeah that would be a little bit more convient. Not sure what you think: So
you think the DynamicRouter should be usable as it is?

On Thu, Oct 8, 2015 at 11:45 AM, Lukas Kahwe Smith <notifications@github.com

wrote:

instead of throwing an exception if we do not have an UrlMatcherInterface,
we duplicate the same logic from the ChainRouter::doMatch() method ..
does that work for you?

—
Reply to this email directly or view it on GitHub
#151 (comment)
.

[lsmith77]

we might be able to solve this via symfony/symfony#16346
but this might mean we decide to bump this lib to 2.0 .. lets see

/cc @Tobion @lolautruche

[dbu]

@dawehner ok for you if you simply do the Request::create for now and we close this issue? the next major version of this library will hopefully drop the match method anyways, as symfony will move to matchRequest with PSR-7 or symfony Request.

[dawehner]

Yeah sure, Drupal will find workarounds, this is for sure