adding candidates subsystem #98
Conversation
dbu
commented
Mar 24, 2014
dbu
commented
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | relates to symfony-cmf/routing-bundle#210 |
| License | MIT |
| Doc PR | - |
|
looks good to me |
|
lets wait with merging until the bundle PR is ready too. i need to see what has to happen for the locales support for example. |
|
added the locales thingy. will still need to further test the bundle. |
| namespace Symfony\Cmf\Component\Routing\Candidates; | ||
|
|
||
| use Doctrine\ODM\PHPCR\DocumentManager; | ||
| use Doctrine\ODM\PHPCR\Query\Builder\QueryBuilder; |
There was a problem hiding this comment.
the PrefixCandidates strategy is actually specific to phpcr - should we move it into the bundle?
|
In contrast to CMF drupal does not put every "object" into the route provider but rather uses patternn instead. We can do that, because our URLs can be aliases, so the internal URL does not really matter. This in general sounds like a nice feature, not sure though whether it is worth to reuse. Based on that, our query strategy to find matching routes is totally different and simple splitting by path won't work. |
|
Okay, was just asking in case you would say "oh we do almost the same and we can merge the behaviour". So for drupal this will just be unused code. |
|
i guess we still wait until the RoutingBundle PR is final, right? |
|
i am pretty confident we have it now. imho we could merge and then still |
|
@dawehner suggests that there might be a DOS risk involved with how we generate candidates. it could make sense in the strategy to limit the number of patterns to attempt to something sensible. should we add a limit and default the limit to 15 or 20 maybe? this is only relevant when you have routes that match on a pattern - we could even configure the limit to 1 when not using patterns in cmf urls at all. |
|
which method specifically should get that limit? |
|
i think the route provider should not know about such details, would put it on the candidates strategy as a global parameter. its really just a sanity check against a potentially heavy request with DOS potential (like |
|
i am still not clear about what should be limited, the possible number of candidates per request I assume? and then in what method would be apply that limit? am I understanding it properly that you want to leave this as the responsibility of the route provider implementations? |
|
yes the number of candidates per request. i would set that on the Candidates strategy and use it in getCandidatesFor. so we would do the limit in the Candidates, not in the route provider - otherwise if you want both with and without locale for example, you would have a real problem. the candidates strategy knows best how to pick the most interesting options. |
|
so you would implement the limit to just pop up the additional candidates (or rather exit early from the loop?) |
|
yeah, exit early from the loop. its not something that should happen in normal operation, but make it harder to overload the system with weird requests. |
|
+1 then |
|
alright, will add that. |
|
okay, pushed a change. lets see if travis is happy |
|
found some mistakes locally, updated. seems travis is not very fast tonight... |
|
did you fix those? seems good to merge to me. |
|
there we go. the RouteBundle PR is not ready yet, i need to cleanup the admin a bit to make it reusable with simplecms admin - there is a huge copy-paste currently. |
