[HttpKernel] Make RouterListener members protected. #4351
Conversation
The $urlMatcher and $logger members that were private are set in the constructor and might be useful for subclasses of RouterListener. Make them protected.
|
Can you provide a link to the code for the RouterListener in Drupal? |
|
Yeah, if new code really requires more extension points, then it's about time to think about creating clean extension points. Changing properties access level doesn't add clean extension points, it just introduces possibility to break things with later updates. |
|
http://drupalcode.org/sandbox/Crell/.../core/lib/Drupal/Core/EventSubscriber/RouterListener.php Looking at the comment on onKernelRequest(), something might be wrong, indeed. |
|
Yup, proper way of solving this might be in adding <?php
...
public function matchRequest(Request $request)
{
return $this->match($request->getPathInfo());
}
...So, we will be able to change this line to <?php
...
public function matchRequest(Request $request)
{
return $this->match($request->attributes->get('system_path'));
}
...This way, Drupal guys will be able to add needed functionality without even touching |
|
@everzet Looks good to me... but without adding that to the interface. Anyone willing to submit a PR with some tests? |
|
It should be on one interface (maybe on a new one), but otherwise you tie the listener to a specific implementation. |
|
Right ... since RouterListener only assumes the interface is implemented we can't just do: However changing UrlMatcherInterface to have both match() and matchRequest() is a bit wierd, too. Does it matter which one I call? How can there be match() with just the URL, if it depends on the whole request? |
|
We cannot change |
|
agree with @schmittjoh, we should define new method in some (new) interface. We should declare new This way, we will save BC and add clean extension point into matcher. |
|
I would suggest |
|
Tried to do this in #4363. |
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).
Drupal 8 is going to have subclasses of HttpKernel and RouterListener (and more). fgm noted - when reviewing the patch to add that - that we were duplicating the $dispatcher and $resolver members of HttpKernel:
The visibility has been set to protected as of 56a4ab7 by @fabpot.
I wonder if doing the same thing for classes like RouterListener makes sense.