Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Routing] Added an optional event trigger for handleRouteRequirements #5651

Closed
wants to merge 7 commits into from
Closed

[Routing] Added an optional event trigger for handleRouteRequirements #5651

wants to merge 7 commits into from

Conversation

@jmather
Copy link

@jmather jmather commented Oct 2, 2012

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: -- as much as they did before -- tests added for feature.
Fixes the following tickets: none
Todo: Documentation
License of the code: MIT
Documentation PR:

Ran into an issue where having a more flexible routing requirements implementation would be helpful, and wanted to see if there was any interest in this.

I added an optional event trigger to handleRouteRequirements to enable more flexible routing rules.

Initially the the use case is when implementing Accept header based routing, but I figured others may want to use that in addition to other custom routing rules, at which point, using the event dispatcher optionally made sense.

You can find my current use-case here: https://gist.github.com/3805361

protected $context;
protected $allow;

/** @var \Symfony\Component\EventDispatcher\EventDispatcher */

This comment has been minimized.

@cordoval

cordoval Oct 2, 2012
Contributor

probably remove this because we all not use phpstorm :)


class UrlMatcherEvent extends Event
{
/** @var \Symfony\Component\Routing\Route */

This comment has been minimized.

@cordoval

cordoval Oct 2, 2012
Contributor

remove

@@ -30,9 +32,14 @@ class UrlMatcher implements UrlMatcherInterface
const REQUIREMENT_MISMATCH = 1;
const ROUTE_MATCH = 2;

const EVENT_HANDLE_REQUIREMENTS = 'match.requirements';

This comment has been minimized.

@stof

stof Oct 2, 2012
Member

routing.match.requirements would be better

protected $context;
protected $allow;

/** @var \Symfony\Component\EventDispatcher\EventDispatcher */
protected $event_dispatcher;

This comment has been minimized.

@stof

stof Oct 2, 2012
Member

Please follow the Symfony coding standards. The name should be camelCased (and btw, many places in symfony are simply using dispatcher as name).

@@ -66,6 +73,14 @@ public function getContext()
}

/**
* @param \Symfony\Component\EventDispatcher\EventDispatcher $event_dispatcher
*/
public function setEventDispatcher(EventDispatcher $event_dispatcher)

This comment has been minimized.

@stof

stof Oct 2, 2012
Member

please typehint the interface. This code would be broken in symfony 2.2 in debug mode as the traceable dispatcher now uses composition instead of inheritance

private $status;

const REQUIREMENT_MISMATCH = 2;
const REQUIREMENT_MATCH = 1;

This comment has been minimized.

@stof

stof Oct 2, 2012
Member

why duplicating the constants here ?

/** @var \Symfony\Component\Routing\Route */
private $route;

/** @var boolean */

This comment has been minimized.

@stof

stof Oct 2, 2012
Member

Please remove this docComment.

And btw, it is wrong. this is not a boolean

/**
* Returns the route being matched
*
* @return \Symfony\Component\Routing\Route

This comment has been minimized.

@stof

stof Oct 2, 2012
Member

Please use a short class name

This comment has been minimized.

@cordoval

cordoval Oct 2, 2012
Contributor

still not implemented

*
* First listener to set to Mismatch wins
*
* @param $result boolean

This comment has been minimized.

@stof

stof Oct 2, 2012
Member

This field is not a boolean but an integer.
and the order is wrong: ``@param integer $result`

@stof
Copy link
Member

@stof stof commented Oct 2, 2012

FrameworkBundle should be updated to use the event dispatcher too.

and I think passing the Route to the event will be broken when using a cached matcher, which is bad. the behavior should be the same when caching the matcher for performances

@jmather
Copy link
Author

@jmather jmather commented Oct 2, 2012

All change suggestions should be implemented now.

Sorry for the code format violations, @stof.

I will look at the caching matcher and see if I can think of anything.

*
* First listener to set to Mismatch wins
*
* @param $status boolean|null

This comment has been minimized.

@cordoval

cordoval Oct 2, 2012
Contributor

integer not boolean right?

This comment has been minimized.

@jmather

jmather Oct 2, 2012
Author

I'm not sure. It is essentially a nullable boolean (true / false / null are the options).

I will leave it to the community to decide it's boolean nature. :D

This comment has been minimized.

@stof

stof Oct 2, 2012
Member

@jmather It is not a boolean. You are setting 1 or 2. A boolean is true or false

This comment has been minimized.

@cordoval

cordoval Oct 2, 2012
Contributor

@stof also said something regarding this that was wasted thanks to Github removing old comments

community is speaking through comments remember :)

/**
* Returns the set status
*
* @return boolean|null

This comment has been minimized.

@cordoval

cordoval Oct 2, 2012
Contributor

same here

@jmather
Copy link
Author

@jmather jmather commented Oct 2, 2012

@stof I am not sure it would break anything (unless I am misunderstanding your comment), as $route is a required argument of handleRouteRequirements so it would have to be passed in order for handleRouteRequirements to run anyway, right?

@@ -16,6 +16,8 @@
use Symfony\Component\Routing\RouteCollection;
use Symfony\Component\Routing\RequestContext;
use Symfony\Component\Routing\Route;
use Symfony\Component\EventDispatcher\EventDispatcherInterface;
use Symfony\Component\EventDispatcher\Event;

This comment has been minimized.

@cordoval

cordoval Oct 2, 2012
Contributor

you can now remove this one i think

@@ -161,6 +174,16 @@ protected function handleRouteRequirements($pathinfo, $name, Route $route)
$scheme = $route->getRequirement('_scheme');
$status = $scheme && $scheme !== $this->context->getScheme() ? self::REQUIREMENT_MISMATCH : self::REQUIREMENT_MATCH;

if (0 === $status) {
return array($status, null);

This comment has been minimized.

@cordoval

cordoval Oct 2, 2012
Contributor

if on the previous line $status is either

self::REQUIREMENT_MISMATCH or
self::REQUIREMENT_MATCH

then why not checking either one of them and doing a check on 0?

This comment has been minimized.

@jmather

jmather Oct 2, 2012
Author

Good point. :) -- Fixing...

@cordoval
Copy link
Contributor

@cordoval cordoval commented Oct 2, 2012

also where are my tests? :'(

@jmather
Copy link
Author

@jmather jmather commented Oct 2, 2012

@cordoval I will make tests once are sure this won't just go into a pr black hole. :)

@cordoval
Copy link
Contributor

@cordoval cordoval commented Oct 2, 2012

i think is already pass that point, @stof had not said this is not needed or unwanted just make your point from the start and fight your PR until the end

@fabpot
Copy link
Member

@fabpot fabpot commented Oct 2, 2012

Big -1 from me as it introduces an (optional) dependency on the event dispatcher (which will lead to more events added over time). The use case you mention is already discussed by Drupal; not sure what they have done on this topic though.

@cordoval
Copy link
Contributor

@cordoval cordoval commented Oct 2, 2012

@fabpot i think this starts to open the quest to see how we are going to integrate content negotiation into symfony2. The drupal guys @lsmith77 @Crell are working on something https://github.com/winmillwill/BadFaith and acknowledge the need also. This is their thread http://drupal.org/node/1505080 . As of now afaik they are attemping something lame and temporary then implement algorithms down the road. So overall there will have to be a way to plug badfaith into symfony2 somehow.

@jmather
Copy link
Author

@jmather jmather commented Oct 2, 2012

@fabpot That was my main concern. I know it adds a dependency. The only other way to implement something like this (that I have found thus far) would be to extend the UrlMatcher, which is fine, until there are two unique things you want to implement, and then you have to piggyback them.

I think it provides a rather clean solution to prevent a lot of ugly code down the line, but I do share your concern about the slippery slope of the dependency.

@fabpot
Copy link
Member

@fabpot fabpot commented Oct 2, 2012

Content negociation should probably be handled by Symfony directly.

@cordoval
Copy link
Contributor

@cordoval cordoval commented Oct 2, 2012

@fabpot, if it is a component maybe it be benefitial or adopted by others, if we force it sf2 only then it would be sad?

@fabpot
Copy link
Member

@fabpot fabpot commented Oct 2, 2012

@cordoval a Symfony Component would be fine.

@jmather
Copy link
Author

@jmather jmather commented Oct 2, 2012

@fabpot even as a symfony component it would still need a way to hook into the routing component. This (or something like it) would provide that hook.

@Crell
Copy link
Contributor

@Crell Crell commented Oct 2, 2012

Given how many half-or-less-implemented negotiation libraries exist, I think it makes more sense for a cross-project negotiation lib to exist. BadFaith was an initial stab at that, but the developer didn't have time to complete it.

Our current thinking in Drupal is to leverage the new NestedMatcher, which lets us plugin whatever matching logic we want as a filter. That just landed in Drupal core yesterday, so the next step there is to push that logic up into the CMF Routing component. One of the partial matchers we need to write then is one that uses a negotiation library to filter a RouteCollection by Accept header. Details of that we're working on now. The coneg library itself IMO should be a free-standing library, since it's really just a PHP port of the Apache mod_negotiation algorithm (or should be).

@jmather
Copy link
Author

@jmather jmather commented Oct 2, 2012

@Crell Do you know if there are there plans (or thoughts of plans) that it would be PR'd to the core? Or would it only be available through the CMF Routing component?

@Crell
Copy link
Contributor

@Crell Crell commented Oct 2, 2012

By "to the core" do you mean the Symfony Routing component, rather than Symfony CMF? There was some discussion elsewhere (I can't find it now) about that, and the general feeling between @fabpot and @dbu was for it to go into the CMF Router, which becomes a sort of "advanced routing toolkit". That way we also don't need to be quite as concerned about breaking existing and widely used interfaces. If fabpot would prefer that it go into the base Routing component, though, I have no strong preference either way.

@jmather
Copy link
Author

@jmather jmather commented Oct 2, 2012

I guess I was meaning in the core, but with @fabpot worried about the slippery slope of event dependance, perhaps just a specific advanced routing package would work as well.

@lsmith77
Copy link
Contributor

@lsmith77 lsmith77 commented Oct 2, 2012

I agree that it would be within the scope of Symfony2 to cover content negotiation. The question is just who will do the work. Its clear that Drupal needs something for D8 and the solution we currently have in FOSRestBundle leads a lot to be desired too.

As for events. This doesnt need to be in the core imho as an equivalent behavior can be achieved by simply extending the class. Obviously if the end goal is to build an "eco system" of logic that consumes these events, getting this into core would facilitate this. However I dont really think there will be so many use cases of this.

And yes as @Crell has pointed out, the CMF Routing component is sort of becoming the place for more "advanced" routing needs, though I guess we will keep it somewhat focused on the dynamic routing use case for CMS.

@jmather
Copy link
Author

@jmather jmather commented Oct 3, 2012

My main thought with having it here in the core is it is that then available for the largest number of uses. If it's pushed out to an auxiliary bundle (even if it's just CMF Routing), many people won't be able to take advantage of it -- either because it's unknown to them or because they're using something else that extends UrlMatcher (or similar) already.

The other side of it is, this need originally arose for me through Silex. I'm not up to speed enough to know if the CMF Routing component can be used with Silex or not, so that may be another point to consider.

@fabpot
Copy link
Member

@fabpot fabpot commented Oct 3, 2012

I have the feeling that having content negociation within Symfony (perhaps as another component or part of HttpFoundation) and making it available in the Router would make the need for custom events almost disappear (at least for legitimate use cases).

@jmather
Copy link
Author

@jmather jmather commented Oct 3, 2012

I agree that having it available in the router would generally eliminiate the need for a feature like this (though I'm sure there are still use cases that would support it -- relying on other things aside from Accept, or basically anything non-content-negotiation related), but we don't have that yet, and @Crell (in his talk) made it seem like it may still be quite a while out.

If CMF Routing is interested in adopting this, then I suppose that would work. It has a wider scope than just CMS implications (as I am actually building it for managing an API), but I can understand not wanting to seemingly open a door to an eventual hard-dependency between routing and the event listener.

@lsmith77
Copy link
Contributor

@lsmith77 lsmith77 commented Oct 3, 2012

think both @fabpot and I agree that support for accept header negotation should best go into Symfony2 core and not the CMF routing component.

yes indeed someone needs to implement it.. which is the biggest issue atm.

@dbu
Copy link
Contributor

@dbu dbu commented Oct 3, 2012

indeed i hope we manage to pull the NestedRouter of drupal into the symfony cmf routing component and improve the DynamicRouter there. with this it will become possible to add any custom routing decision logic. once this lands, you can add some temporary content negotiation solution to it until symfony core will have content negotiation. the need for a general hook is small because people can use the cmf routing component if they have some specific requirement.

@Crell
Copy link
Contributor

@Crell Crell commented Oct 3, 2012

The way NestedMatcher is implemented, there are no events. It's just a series of filters registered via a method on the main matcher, in practice via the Container in most cases. However, we are still debating the potential performance concerns of that, as you could, conceptually, run into a lot of partial matchers that apply only to certain routes, but get called for all routes anyway. How to resolve that without a dependency on events or a ContainerAware matcher object is something we're still debating.

@jmather
Copy link
Author

@jmather jmather commented Oct 3, 2012

I could replace this with a more locked down specific filter implementation instead of using the event dispatcher, if that would help. It'd be a quick switch, I think.

I just assumed we already had a system for doing things like this, and so reusing it would be better than baking in another basic iteration-over-an-array decision system.

It also makes such a solution easier to adopt, as it follows an already established pattern.

I agree with @Crell that if handled poorly there are performance implications, but almost everything has a down side if people use it inappropriately.

@fabpot
Copy link
Member

@fabpot fabpot commented Oct 3, 2012

@jmather First, we need the real-world use cases (that should not be implemented in another way). Then, the proposed solution must work for all implementations of the matcher (Apache, the compiled router, ...). Finally, the performance are very important for this component. So, let's first discuss the use cases before even thinking about the implementation.

@lsmith77 How can we move forward? Does someone need help with the content negociation implementation? Do we need to start from scratch?

@jmather
Copy link
Author

@jmather jmather commented Oct 3, 2012

@fabpot: fair enough. Where should we collect the use cases? Is there a process for it?

@Crell
Copy link
Contributor

@Crell Crell commented Oct 3, 2012

There's some existing discussion of use cases here: http://groups.drupal.org/node/256028 with some follow-up discussion here: http://drupal.org/node/1784040

The examples are somewhat Drupal-specific, but the logic questions are not. Y'all are welcome to join us in the discussion as there's little that's Drupal-specific about it. (Hence why we want to avoid a Drupal-specific solution.)

@lsmith77
Copy link
Contributor

@lsmith77 lsmith77 commented Oct 3, 2012

@fabot someone just needs to write the code. i just dont have the time but my original suggestion was to reimplement the mod_negotiation algorithm. this is what was attempted but never completed in the BadFaith lib.

@fabpot
Copy link
Member

@fabpot fabpot commented Oct 3, 2012

@winmillwill Are you still willing to work on BadFaith? If not, I'd like to take over, move the code into Symfony, and finish the implementation.

@cordoval
Copy link
Contributor

@cordoval cordoval commented Oct 3, 2012

👍 don't forget to pull also the pending PRs :)

name is src\Symfony\Component\ContentNegotiation

@fabpot
Copy link
Member

@fabpot fabpot commented Oct 3, 2012

@cordoval There are not so many PR waiting for me. A lot of them still cannot be merged yet.

@winmillwill
Copy link

@winmillwill winmillwill commented Oct 4, 2012

Not working on this anymore, please have at it, and thanks everyone for taking over.

@Crell
Copy link
Contributor

@Crell Crell commented Oct 4, 2012

@fabpot: If you're working on the coneg library, be aware of this webkit bug we stumbled across in Drupal: http://drupal.org/node/1716790

That's going to mess with the proper ordering of accept priorities.

@fabpot
Copy link
Member

@fabpot fabpot commented Oct 27, 2012

Closing this PR for the reasons explained in the comments. Content negotiation is being worked on in other PRs.

@fabpot fabpot closed this Oct 27, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants