Skip to content

[WIP] ORM based dynamic routing #95

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

Closed

Conversation

matteocaberlotto
Copy link
Contributor

This should be the code to start from

@matteocaberlotto
Copy link
Contributor Author

I wont have much time next month, if there's no hurry i can keep on working on it.
left todo

  • check coexistence phpcr with orm
  • implement redirection
  • removed duplicated/unused code

there's something wrong with functional tests setup i need to figure out

@dbu
Copy link
Member

dbu commented May 22, 2013

cool, thanks! i will have a good look at this and try to reduce code duplication. will ask a few questions in the code now.

there are some code style issues and such, but lets look at those in the end.

@@ -45,6 +45,9 @@ public function getConfigTreeBuilder()
->fixXmlConfig('locale')
->children()
->scalarNode('enabled')->defaultNull()->end()
->arrayNode('instances')
->prototype('scalar')->end()
->end()
Copy link
Member

Choose a reason for hiding this comment

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

from the code below i understand you moved the route enhancer configuration under this node. what exactly for? to allow more than one dynamic router being configured at the same time? we could do that, but then we should move the information if its to be an orm or phpcr-odm router in there as well. how do you decide that right now? or do you have the extension load and enhance a dynamic router service defined by your application?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think you know better how to handle this...

i added those lines as child of 'dynamic' field of 'symfony_cmf_routing' (or 'cmf_..') into app/config/config.yml

    instances:
        - symfony_cmf_routing.orm.dynamic_router
        - symfony_cmf_routing.dynamic_router

the chain -> routers_by_id tells which router and priority, they are not all dynamic one but the dynamic should be setup by the extension if i understood correct.
should another field (say 'dynamic') be added to 'routers_by_id' to mark a router as dynamic? the provider info are into xml services config and this tells which repository should the router fetch from.

Copy link
Member

Choose a reason for hiding this comment

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

as solution for this issue, #112 is separating the route provider from the rest of the dynamic router. the configuration does not expect that you would want to run more than one dynamic router. i think that makes sense, people in most cases won't need both orm and phpcr routes at the same time. if they really do, they can still define their own dynamic router service in their application.

@dbu
Copy link
Member

dbu commented May 23, 2013

thanks a lot for this so far! i think we are quite on track.
i will take this PR and refactor some things in the bundle, then create a PR against your branch from that. the phpcr-odm stuff is doing some not elegant things that lead to too much code duplication.

@matteocaberlotto
Copy link
Contributor Author

thanks you all too

@erichard
Copy link

Is someone still working on this PR ? I've started working on the ORM implementation too so maybe we can collaborate on it.

@dbu
Copy link
Member

dbu commented Jun 10, 2013

hi @erichard i started sorting things out, but got sidetracked by other things. steps to take imo are:

  1. move Document/* classes to Model/ and use the mapping compiler pass for that folder. check if we need a Doctrine/Phpcr/ class extending the class in Model or if all fields of the class are storage agnostic
  2. add orm mappings and check if we need Doctrine/Orm classes or if the generic models have all fields we need.

so essentially i want to keep database specific code to a minimum. the provider itself will be specific, and the xml mappings. if you want to work on 1, the essential documentation is in symfony/symfony-docs#2669

@dbu dbu mentioned this pull request Jun 14, 2013
@dbu
Copy link
Member

dbu commented Jun 14, 2013

i just created #112 with the refactoring i had in mind. once that is merged, it should be a manageable junk of code to port from this PR into the new way of doing things. trickiest part is probably the bundle configuration.

@dbu
Copy link
Member

dbu commented Jul 19, 2013

see #122

@dbu dbu closed this Jul 19, 2013
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.

4 participants