-
Notifications
You must be signed in to change notification settings - Fork 77
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
Added child restriction to Route & RedirectRoute documents #389
Conversation
@@ -13,6 +13,7 @@ | |||
<nodename name="name"/> | |||
<parent-document name="parent"/> | |||
<children name="children"/> | |||
<child-class name="Symfony\Cmf\Component\Routing\RouteObjectInterface"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a couple of options here:
- Restrict to this interface
- Restrict to
Symfony\Component\Routing\Route
- Restrict to the
Route
andRedirectRoute
models of this bundle
I choose the first option, as it seems to be the most flexible without completely allowing anything.
b29b694
to
02a2349
Compare
02a2349
to
a0d34bc
Compare
@dbu this test seems to suggest that adding a non-route document as child of a route is supported: https://github.com/symfony-cmf/routing-bundle/blob/master/tests/Functional/Doctrine/Phpcr/RouteProviderTest.php Should we allow all childs and only keep using the route filtering in the RouteProvider, or should we remove the filtering in the provider and rely on child restrictions? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am not sure if we can do this actually. the tree structure translates to urls - the other bundles are only internal representation.
we could have something like /blog/2017/02/09/my-post where 2017, 02 and 09 are autocreated and empty. or redirect to the parent or whatever. maybe check what things the RoutingAutoBundle supports to see use cases we currently have in the cmf itself.
if redirect routes can have route children, i think we could say this just forces to use a good route design (place redirect routes as intermediate, rather than non-routes that lead to a 404 when removing folders from the url). as the mapping can be overwritten, that sounds like a decision we can promote.
@@ -5,7 +5,7 @@ | |||
https://github.com/doctrine/phpcr-odm/raw/master/doctrine-phpcr-odm-mapping.xsd" | |||
> | |||
|
|||
<document name="Symfony\Cmf\Bundle\RoutingBundle\Doctrine\Phpcr\RedirectRoute"> | |||
<document name="Symfony\Cmf\Bundle\RoutingBundle\Doctrine\Phpcr\RedirectRoute" is-leaf="true"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i disagree with this. we could have an intermediate node redirect to the default child node. as in /topics redirecting to /topics/symfony (ok, not the best example but i think there are valid use cases like this)
Currently, routing auto uses the Generic document for the intermediate parts. Do we want to switch that to redirect routes? (and with what behaviour, i.e. redirect to the first non-redirect parent node?) /cc @dantleech Or we should skip this for the RoutingBundle, think about this a little more and add this in CMF 3.0. |
Closing as we are not sure how to apply this to routes. |
/fixes #386
Builds are failing because of a bug in PHPCR ODM 1.4.0. The fix is already in master, but not yet released as 1.4.1.