symfony-cmf / routing-auto

RoutingAuto component
https://cmf.symfony.com
Other
6 stars 11 forks source link

Intermediate PHPCR generic nodes issue #61

Closed mmenozzi closed 8 years ago

mmenozzi commented 9 years ago

Assume the following situation. cmf_routing_auto.yml as follows:

AppBundle\Document\News:
    uri_schema: /news/{title}
    token_providers:
        title: [content_method, { method: getTitle }]
    conflict_resolver: auto_increment
    defunct_route_handler: leave_redirect

AppBundle\Document\Page:
    uri_schema: /{title}
    token_providers:
        title: [content_method, { method: getTitle }]
    conflict_resolver: auto_increment
    defunct_route_handler: leave_redirect

Assume that with this configuration I first create a "News" document whose title is "My News". Assuming also that the routes base path is /cms/routes I have the following PHPCR nodes:

Now if I want to create a Page whose title is "News" I'll get the following error:

[1/2] ContextErrorException: Catchable Fatal Error: Argument 1 passed to Symfony\Cmf\Bundle\RoutingAutoBundle\Adapter\PhpcrOdmAdapter::compareAutoRouteContent() must implement interface Symfony\Cmf\Component\RoutingAuto\Model\AutoRouteInterface, instance of Doctrine\ODM\PHPCR\Document\Generic given ...

This is because the schema configuration for a Page document whose title is "News" will end in the PHPCR node path /cms/routes/news and this node already exists and is an instance of Doctrine\ODM\PHPCR\Document\Generic.

I think that the RoutingAuto component should treat a Doctrine\ODM\PHPCR\Document\Generic node as a "free" route and should replace the generic node with an AutoRoute node that point to the proper content document.

What do you think about this? Can I work on a PR on this issue?

dantleech commented 9 years ago

I agree with what you suggest.

Assuming that we will only ever have instances of Route and Generic in the route tree, then we could safely assume that Generic nodes can be replaced by Route nodes and throw an exception if any other node type were found.

One problem we will encounter is how to "replace" the Generic node, as we are also concerned with its children.

This work could be done in the AutoRouteManager https://github.com/symfony-cmf/RoutingAuto/blob/1.0.1/AutoRouteManager.php#L60

This should probably be done in the adapter, using a new method replaceRoute, and the logic called from the AutoRouteManager.

So, please feel free to have a go at this :)

mmenozzi commented 9 years ago

Hi Daniel, Yes you're right, this is a PHPCR adapter issue because in an ORM adatpter we probably wouldn't have this problem. So I think I opened the issue in the wrong place, sorry... Anyway I think that we have not to change the AutoRouteManager implementation nor the AdapterInterface. I think it's sufficent to change the implementation of two methods in the PHPCR adapter:

What do you think?

dantleech commented 9 years ago

Yes, that sounds good :) If we use this solution then we should remember to document these new responsiblities in the AdapterInterface in this library.

wouterj commented 9 years ago

@ I don't think it's a new responsibility. It's just a new implementation of the current responsibilities for the PHPCR ODM adapter.

mmenozzi commented 9 years ago

I agree with @WouterJ. I don't see it as a new responsibility of the Adapter. Anyway I'll work on it and I'll come back with a PR (I hope).

mmenozzi commented 8 years ago

@dantleech @WouterJ PR submitted. Let me know what do you think. Thank you!