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

[WIP] candidates strategy #213

Closed
wants to merge 5 commits into from
Closed

[WIP] candidates strategy #213

wants to merge 5 commits into from

Conversation

dbu
Copy link
Member

@dbu dbu commented Feb 1, 2014

this is just a quick shot to show how the candidates strategy could look. /cc @benglass

done against #210 to show the additions.

we miss constructor and bootstrap adjustments here.

@@ -203,16 +122,9 @@ private function getAllRoutes()
$dm = $this->getObjectManager();
$sql2 = 'SELECT * FROM [nt:unstructured] WHERE [phpcr:classparents] = '.$dm->quote('Symfony\Component\Routing\Route');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we really should use the querybuilder here. @dantleech does fromDocument use the phpcr:classparents too or only phpcr:class?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this work? $qb->from()->document('Symfony\Comonent\Routing\Route')

@dbu
Copy link
Member Author

dbu commented Mar 15, 2014

@benglass i am picking this up again. i quite like this candidates thing. it allows to encapsulate the prefix stuff into one place, plus the listeners that update stuff which are specific to the prefix strategy.

do you think this can work? can you imagine how it would help with multisite? i will try to wrap things up and get simplecms to run in the sandbox with this. the normal routing is already good now, just a few glitches left.

}
if ($prefixConstraints) {
$sql2 .= ' AND (' . implode(' OR ', $prefixConstraints) . ')';
$candidateConstraint = $this->candidatesStrategy->getQueryRestriction();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbu I am wondering if the candidates strategy is the right place for the responsibility of building the query restriction. It seems that it might be better for the route provider to build the query restriction based on the returned candidates since it already knows abut the PhpcrQueries.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the idea is that the provider no longer knows about prefixes at all. for all it knows it could be some other strategy. or the prefix logic could be more complicated, like excluding some sub path. i will try to use the query builder instead of this string concat thing, then it should look nicer.

@benglass
Copy link
Member

@dbu Overall I would say this looks like a good approach and for multi site in our implementation we would just use multiple prefixes (which the candidate strategy approach supports).

I am trying to think if this would cause issues if you had 2 prefixes that had the same paths in them (for example a common path like /about might appear in /cms/main and /cms/private prefixes). I will need to read the code more carefully with this in mind to try to determine if this would be an issue.

@benglass
Copy link
Member

@dbu regarding multi-host links where the url part is non unique (such as /about) i am not sure this approach will work. You could use the getCandidates(Request $request) method to only get candidates for the matching host however this would cause issues when trying to generate URLs for a path on host b when you are viewing a page on host a because you could not safely scope the request to only include prefixes for the current host.

Possibly a solution to this is not to generate URLs using paths, but instead to use the uuid but this seems a workaround. Thoughts?

@dbu
Copy link
Member Author

dbu commented Mar 15, 2014

thanks for the feedback ben. about the duplicates: this is simply a question of the order of the prefixes. there is always room for collisions as soon as we have more than one routing mechanism.

we could add a priority to PrefixCandidates::addPrefix. that would allow to add the domain specific prefix as first. or we could ask to do getPrefixes(), array_unshift, setPrefixes(). what do you think?

@benglass
Copy link
Member

@dbu im not sure the priority would fix the issue, here is a scenario

You are on http://www.host1.com and you want to link to http://www.host2.com/about

Lets assume the prefix for host1.com is /cms/host1 and /cms/host2 for host.com and that both hosts have a /about route (both /cms/host1/about and /cms/host2/about exist)

How do we link from a page on host1.com to host2.com/about?

This is a problem that is specific to url generation and not relevant to routing (when we need to translate the url /about to a path we can use the host to look up the right prefix in the candidate strategy).

Do you see the issue I am trying to illustrate?

Url generation for cmf paths are somewhat volatile anyway and the normal behavior of throwing a RouteNotFound exception is not desirable (when the page is linked to from cms content for example). Perhaps this is a separate issue? We had discussed having separate cmf_path and cmf_url methods that would handle this but it does create a gotcha if you cannot generate a url to a route via the normal twig path methods.

@dbu
Copy link
Member Author

dbu commented Mar 16, 2014

ah, now i see what you mean. but this is really a multisite question how to determine the route name to generate. cmf route names are /cms/host1/about, or you use the object which also knows which host it is. the interesting bit will be how you make the route generate understand it needs to generate www.host1.com/about when it runs on host2, but i think this has to go somewhere else - quickly tried to figure out how that normally works but did find nothing.

what we will need is another listener that configures the host requirement, as we now have the listener for idprefix or locale. the goal of this PR is to make it easier to later do multisite, not to already implement it.

@benglass
Copy link
Member

@dbu undertstood regarding the goal of the PR, Im just trying to think through how the multi site routes will work. I think your suggestion of a doctrine post load listener which sets a host requirement on the route after it is loaded is probably the correct approach.

I am fairly sure that with a host requirement set, the route will be generated with an absolute url if the host does not match the current host. I will look into this further but as you mentioned it shouldn't necessarily effect this PR.

@dbu
Copy link
Member Author

dbu commented Mar 17, 2014

thanks a lot for the feedback. i will try to wrap this thing up tonight!

@dbu
Copy link
Member Author

dbu commented Mar 24, 2014

8f3e4f9 has been cherry-picked into #210

@dbu dbu closed this Mar 24, 2014
@dbu dbu deleted the route-candidates branch April 4, 2014 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants