[Routing][HttpKernel] Add RequestMatcherInterface. #4363

niklasf · 2012-05-21T18:54:48Z

First try at implementing #4351.

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Fixes the following tickets: -
Todo: -
License of the code: MIT

travisbot · 2012-05-21T19:37:07Z

This pull request fails (merged 457496db into ea33d4d).

everzet · 2012-05-21T19:47:04Z

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

+            if ($this->urlMatcher instanceof RequestMatcherInterface) {
+                $parameters = $this->urlMatcher->matchRequest($request);
+            }
+            else {


should be on previous line, right after }

Heh, Symfony coding standards. Thanks for reviewing. Should I amend my previous commit or push the fix in a seperate one?

amend please

Done and pushed.

travisbot · 2012-05-21T19:47:51Z

This pull request passes (merged 78effa98 into ea33d4d).

everzet · 2012-05-21T20:17:03Z

No tests?

travisbot · 2012-05-21T20:18:18Z

This pull request passes (merged 6564fb6a into ea33d4d).

schmittjoh · 2012-05-21T20:35:14Z

You need to remove the type-hint from the constructor, and probably add an exception instead where the matching methods are called to ensure that either UrlMatcherInterface, or RequestMatcherInterface were passed. Alternatively, you could also add such a check to the constructor.

schmittjoh · 2012-05-21T20:37:11Z

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

Is there a benefit in changing this file? I think we could revert all of these changes.

Not required at all and does nothing new. That would just give us a default implemenation. Implementations that override this could just as well implement RequestMatcherInterface themselves. Any other votes?

fabpot · 2012-05-22T06:52:01Z

related to #4020

niklasf · 2012-05-25T11:11:45Z

Reverted the changes to UrlMatcher.php.

fabpot · 2012-05-25T12:46:06Z

@niklasf: it looks good now except for the listener constructor (see @schmittjoh suggestion above). Can you fix that and add some unit tests to ensure that everything works as expected? Thanks.

stof · 2012-05-25T12:52:59Z

Another solution could be to make the RequestMatcherInterface extend the MatcherInterface to keep the typehint in the constructor

fabpot · 2012-05-25T13:52:26Z

I thought about that as well, but that does not make sense.

stof · 2012-05-25T14:12:19Z

Well, the RouterInterface extends UrlMatcherInterface anyway (and it should stay this way as it would be a huge BC break) so I guess most people will implement both interfaces when implementing the RquestMatcherInterface

niklasf · 2012-05-25T15:24:38Z

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

@@ -32,8 +33,13 @@ class RouterListener implements EventSubscriberInterface
    private $urlMatcher;


Did that and added some basic tests. This one is private --- should this be renamed to matcher?

Yes, renaming to matcher is a good idea.

travisbot · 2012-05-25T15:26:22Z

This pull request fails (merged 8f36204c into ff4d446).

travisbot · 2012-05-25T15:33:13Z

This pull request fails (merged 6d2f2cd9 into ff4d446).

travisbot · 2012-05-25T15:39:01Z

This pull request fails (merged 3c1d89e2 into ff4d446).

niklasf · 2012-05-25T15:48:10Z

src/Symfony/Component/HttpKernel/Tests/EventListener/RouterListenerTest.php

@@ -52,6 +52,19 @@ public function testPort($defaultHttpPort, $defaultHttpsPort, $uri, $expectedHtt
        $this->assertEquals($expectedHttpPort, $context->getHttpPort());
        $this->assertEquals($expectedHttpsPort, $context->getHttpsPort());
        $this->assertEquals(0 === strpos($uri, 'https') ? 'https' : 'http', $context->getScheme());
+
+        $requestMatcher = $this->getMockBuilder('Symfony\Component\Routing\Matcher\RequestMatcherInterface')


Mhh ... probably picked the wrong test method - should add a new one. Not sure why the tests are failing ...

for an interface, you could use $this->getMock('...') directly. Disablign the original constructor is useless for interfaces as they don't have one

btw, you should probably create another test method for the case of the RequestMatcherInterface instead of putting all assertions in the same test

Thanks, stof. Pushed a fix.

travisbot · 2012-05-25T22:06:34Z

This pull request fails (merged 3ab997c1 into ff4d446).

travisbot · 2012-05-25T22:06:47Z

This pull request fails (merged d8c0e387 into ff4d446).

fabpot · 2012-05-26T06:41:11Z

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

+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ *
+ * @api


Can you remove the @api tag here and below. We will mark them later on. Thanks.

fabpot · 2012-05-26T06:41:31Z

Can you add a note in the CHANGELOG of the component?

travisbot · 2012-05-26T08:12:40Z

This pull request fails (merged c7458733 into 9e95199).

niklasf · 2012-05-26T08:14:41Z

@fabpot: Sorry, not sure how: Under 2.1.0 or in a new section? As the first or last entry of the list?

stof · 2012-05-26T10:20:23Z

@niklasf the new interface should be mentioned in the changelog of the Routing component

niklasf · 2012-05-26T10:28:03Z

src/Symfony/Component/Routing/CHANGELOG.md

@@ -4,6 +4,7 @@ CHANGELOG
 2.1.0
 -----

+ * added RequestMatcherInterface


I guess I should just try it and you'll tell me where to move it, if it's wrong here.

travisbot · 2012-05-26T10:30:06Z

This pull request fails (merged 34ea86a9 into 9e95199).

niklasf · 2012-06-02T15:27:01Z

Ah ... so there were two pitfalls:

PHPUnit clones the arguments of mocked functions. So they wouldn't equal.
createGetResponseEventForUri() disables routing on purpose. So not using that helper, now.

Tests should be passing.

travisbot · 2012-06-02T15:30:28Z

This pull request passes (merged 7fab0118 into 1541fe2).

While UrlMatcherInterface is only for matching URLs with routes, add RequestMatcherInterface, that allows to match against whole requests.

@niklasf

Commits ------- 2277500 [Routing][HttpKernel] Add RequestMatcherInterface. Discussion ---------- [Routing][HttpKernel] Add RequestMatcherInterface. First try at implementing #4351. Bug fix: no Feature addition: yes Backwards compatibility break: no Fixes the following tickets: - Todo: - License of the code: MIT --------------------------------------------------------------------------- by travisbot at 2012-05-21T19:37:07Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1392716) (merged 457496db into ea33d4d). --------------------------------------------------------------------------- by travisbot at 2012-05-21T19:47:51Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1392939) (merged 78effa98 into ea33d4d). --------------------------------------------------------------------------- by everzet at 2012-05-21T20:17:03Z No tests? --------------------------------------------------------------------------- by travisbot at 2012-05-21T20:18:18Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1393392) (merged 6564fb6a into ea33d4d). --------------------------------------------------------------------------- by schmittjoh at 2012-05-21T20:35:14Z You need to remove the type-hint from the constructor, and probably add an exception instead where the matching methods are called to ensure that either ``UrlMatcherInterface``, or ``RequestMatcherInterface`` were passed. Alternatively, you could also add such a check to the constructor. --------------------------------------------------------------------------- by fabpot at 2012-05-22T06:52:01Z related to #4020 --------------------------------------------------------------------------- by niklasf at 2012-05-25T11:11:45Z Reverted the changes to UrlMatcher.php. --------------------------------------------------------------------------- by fabpot at 2012-05-25T12:46:06Z @niklasf: it looks good now except for the listener constructor (see @schmittjoh suggestion above). Can you fix that and add some unit tests to ensure that everything works as expected? Thanks. --------------------------------------------------------------------------- by stof at 2012-05-25T12:52:59Z Another solution could be to make the ``RequestMatcherInterface`` extend the ``MatcherInterface`` to keep the typehint in the constructor --------------------------------------------------------------------------- by fabpot at 2012-05-25T13:52:26Z I thought about that as well, but that does not make sense. --------------------------------------------------------------------------- by stof at 2012-05-25T14:12:19Z Well, the RouterInterface extends UrlMatcherInterface anyway (and it should stay this way as it would be a huge BC break) so I guess most people will implement both interfaces when implementing the RquestMatcherInterface --------------------------------------------------------------------------- by travisbot at 2012-05-25T15:26:22Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1433963) (merged 8f36204c into ff4d446). --------------------------------------------------------------------------- by travisbot at 2012-05-25T15:33:13Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1433996) (merged 6d2f2cd9 into ff4d446). --------------------------------------------------------------------------- by travisbot at 2012-05-25T15:39:01Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1434060) (merged 3c1d89e2 into ff4d446). --------------------------------------------------------------------------- by travisbot at 2012-05-25T22:06:34Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1437398) (merged 3ab997c1 into ff4d446). --------------------------------------------------------------------------- by travisbot at 2012-05-25T22:06:47Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1437385) (merged d8c0e387 into ff4d446). --------------------------------------------------------------------------- by fabpot at 2012-05-26T06:41:31Z Can you add a note in the CHANGELOG of the component? --------------------------------------------------------------------------- by travisbot at 2012-05-26T08:12:40Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1440435) (merged c7458733 into 9e95199). --------------------------------------------------------------------------- by niklasf at 2012-05-26T08:14:41Z @fabpot: Sorry, not sure how: Under 2.1.0 or in a new section? As the first or last entry of the list? --------------------------------------------------------------------------- by stof at 2012-05-26T10:20:23Z @niklasf the new interface should be mentioned in the changelog of the Routing component --------------------------------------------------------------------------- by travisbot at 2012-05-26T10:30:06Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1440837) (merged 34ea86a9 into 9e95199). --------------------------------------------------------------------------- by niklasf at 2012-06-02T15:27:01Z Ah ... so there were two pitfalls: - PHPUnit clones the arguments of mocked functions. So they wouldn't equal. - createGetResponseEventForUri() disables routing on purpose. So not using that helper, now. Tests should be passing. --------------------------------------------------------------------------- by travisbot at 2012-06-02T15:30:28Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1509054) (merged 7fab0118 into 1541fe2).

niklasf mentioned this pull request May 21, 2012

[HttpKernel] Make RouterListener members protected. #4351

Closed

everzet reviewed May 21, 2012
View reviewed changes

schmittjoh reviewed May 21, 2012
View reviewed changes

niklasf reviewed May 25, 2012
View reviewed changes

fabpot reviewed May 26, 2012
View reviewed changes

niklasf reviewed May 26, 2012
View reviewed changes

[Routing][HttpKernel] Add RequestMatcherInterface.

2277500

While UrlMatcherInterface is only for matching URLs with routes, add RequestMatcherInterface, that allows to match against whole requests.

fabpot merged commit 2277500 into symfony:master Jun 9, 2012

Tobion mentioned this pull request Nov 2, 2012

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

Closed

5 tasks

fabpot mentioned this pull request Aug 9, 2013

make RequestContext optional in generator and matcher #8705

Closed

		@@ -32,8 +33,13 @@ class RouterListener implements EventSubscriberInterface
		private $urlMatcher;

Uh oh!

[Routing][HttpKernel] Add RequestMatcherInterface. #4363

[Routing][HttpKernel] Add RequestMatcherInterface. #4363

Uh oh!

Conversation

niklasf commented May 21, 2012

Uh oh!

travisbot commented May 21, 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!

travisbot commented May 21, 2012

Uh oh!

everzet commented May 21, 2012

Uh oh!

travisbot commented May 21, 2012

Uh oh!

schmittjoh commented May 21, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented May 22, 2012

Uh oh!

niklasf commented May 25, 2012

Uh oh!

fabpot commented May 25, 2012

Uh oh!

stof commented May 25, 2012

Uh oh!

fabpot commented May 25, 2012

Uh oh!

stof commented May 25, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

travisbot commented May 25, 2012

Uh oh!

travisbot commented May 25, 2012

Uh oh!

travisbot commented May 25, 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!

travisbot commented May 25, 2012

Uh oh!

travisbot commented May 25, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented May 26, 2012

Uh oh!

travisbot commented May 26, 2012

Uh oh!

niklasf commented May 26, 2012

Uh oh!

stof commented May 26, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

travisbot commented May 26, 2012

Uh oh!

niklasf commented Jun 2, 2012

Uh oh!

travisbot commented Jun 2, 2012