move document to model #112
Conversation
| @@ -48,7 +48,7 @@ protected function updateId(LifecycleEventArgs $args) | |||
|
|
|||
| // only update route objects and only if the prefix can match, to allow | |||
| // for more than one listener and more than one route root | |||
| if ($doc instanceof Route | |||
| if (($doc instanceof Route || $doc instanceof RedirectRoute) | |||
There was a problem hiding this comment.
this is one of the drawbacks of not having multiple inheritance/traits to build the document classes...
There was a problem hiding this comment.
Not sure I understand, why can't they at least implement an interface?
There was a problem hiding this comment.
well then we would define a PhpcrPathPrefixInterface or something, just sounds weird as its just an implementation detail. if this was a Trait, we could check the class for having the trait. (but until php 5.3 can be dropped, i guess we are at least in Symfony3...)
There was a problem hiding this comment.
Hmm. Why would such an interface be a bad idea? Although, the only problem I can see here is if/when the user wants another type of Routing object à la "RedirectRoute". They would have to override this class - creating a little interface in the Doctrine/PHPCR namespace seems like a painless solution?
If we were to implement the interface I guess we would also have to determine the ID from either the DM or the metadata also.
There was a problem hiding this comment.
ok, convinced. i added the interface
|
just pushed a fix for the configuration that will solve #105 . now this should be ready, and ready for adding the orm routing. |
| * | ||
| * @param SymfonyRoute $document the redirection target route | ||
| */ | ||
| public function setRouteTarget(SymfonyRoute $document) |
There was a problem hiding this comment.
Should this be Route and not SymfonyRoute? What would be the point of assigning a non-persisted route object?
There was a problem hiding this comment.
no, this is on purpose. it needs to be something extending SymfonyRoute but not necessarily extending our model class. if its not mapped, phpcr-odm will complain when flushing.
|
We should implement the testing component on this bundle after this merge I think. |
|
we do have quite some functional tests and the phpcr-odm bootstrap in this bundle already. but yeah, the testing component would clean up things and make it consistent with the other bundles. |
|
@erichard @matteocaberlotto would be awesome if you could review this PR. the idea is to make supporting ORM simple with no code duplication. there is also a preliminary ORM PR #116 - that would be the place the ORM specific code that the original PR by matteocaberlotto contained. |
|
hi, i will try to do it asap. i'm sorry, i am on time shortage lately. |
| @@ -0,0 +1,16 @@ | |||
| <doctrine-mapping | |||
There was a problem hiding this comment.
Why is this RedirectRoute.phpcr.xml? Should this not be in the doctrine-phpcr directory and not in doctrine-model?
There was a problem hiding this comment.
no, we map the RedirectRoute model for the phpcr doctrine. doctrine-phpcr is for the classes in Doctrine/Phpcr that extend those in Model
|
the configuration part is working fine: now it only requires the specific provider to add orm routing. |
Conflicts: CHANGELOG.md DependencyInjection/CmfRoutingExtension.php Tests/Functional/Doctrine/Phpcr/RedirectRouteTest.php Tests/Functional/Doctrine/Phpcr/RouteTest.php
This turned out quite a big change in terms of code. in terms of deprecation, the only change for the user is he should now use Doctrine\Phpcr\Route instead of Document\Route.
SimpleCmsBundle will need adjustments as i did not do BC classes for the route and content providers.
Note that i did not yet change the configuration in a way that lets us chose the storage layer - that will be the next step when porting the things from #95 to this new concept.