Skip to content
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

Registry #31

Closed
wants to merge 9 commits into from
Closed

Registry #31

wants to merge 9 commits into from

Conversation

stof
Copy link
Member

@stof stof commented Jul 8, 2011

This implements a registry like in DoctrineBundle (but without the reset() method).

This makes the use of the form type easier: you don't need to pass the document manager in each nested level of the form until you reach the document type if you don't use the default document manager. It also allows suppporting multiple document managers in the type guesser (and I added the type guesser in the extension to allow registering it easily outside Symfony).
The UniqueValidator is not converted to the registry yet.

@stof
Copy link
Member Author

stof commented Jul 28, 2011

Any chance for this to get merged ? If yes I will do the little changes needed in the PR (same fixes than the one I did in DoctrineBundle after this PR).

We could also do it a bit differently by using the way @beberlei used in DoctrineCouchDBBundle and that he hope to be applied in DoctrineBundle too if possible to share the interface in 2.1.

@kriswallsmith
Copy link
Contributor

I would like to merge this. Is it ready?

@beberlei
Copy link
Member

I would rather see the naming of the CouchDB Bundle here as well. That way we already have 2 of 3 bundles with a common syntax and start sharing the Form code. just the naming would be enough for the beginning i guess, i will poke fabien about how we go and solve this issue for 2.1 then.

@stof
Copy link
Member Author

stof commented Aug 19, 2011

@fabpot What do you think about @beberlei's proposal for DoctrineBundle ? This would then impact the decision about how this PR should be updated.

@lsmith77
Copy link
Member

is the intention to then also move the PersistentObjectRegistry into the DoctrineBundle or some other place that allows easy sharing?

@stof
Copy link
Member Author

stof commented Aug 19, 2011

@lsmith77 probably in the Doctrine Bridge

1 similar comment
@stof
Copy link
Member Author

stof commented Aug 19, 2011

@lsmith77 probably in the Doctrine Bridge

@fabpot
Copy link
Member

fabpot commented Aug 19, 2011

Why not moving a Registry-like class in Doctrine itself?

@stof
Copy link
Member Author

stof commented Aug 19, 2011

@fabpot Because all this stuff is about the Symfony integration. Doctrine does not mind at all if you have multiple entity managers or not.

@fabpot
Copy link
Member

fabpot commented Aug 19, 2011

@stof: If Doctrine does not mind, then Symfony doesn't mind either. It's not about Symfony integration at all. It's about providing ways to get information about your configured entity managers, connections, ... It's really useful even if you don't use Symfony. IIRC, Hibernate has such a concept. The current implementation is tied to the Symfony DIC, but it does not have to be. @beberlei: what do you think?

@stof
Copy link
Member Author

stof commented Aug 19, 2011

Doctrine currently does not mind at all how you save your entity manager instance to access it again, Symfony does. The registry is mainly used to support multiple entity managers in the integration with the Form and Validator components currently.
Thinking about it again, it could be a good idea to have the interface in Doctrine Common so that it can be reused (for instance for Silex with a Pimple-based registry). But this would mean that we would have to wait the 2.2 release to have it, and then bump the requirement on Common from 2.1 to 2.2, and so that Symfony 2.1 cannot be released before Doctrine 2.2 (which is planned for December IIRC)

@fabpot
Copy link
Member

fabpot commented Aug 19, 2011

@stof: my point is that I don't want to redo what we've done in symfony1, where the Doctrine plugin was really about implementing another ORM on top of Doctrine1. If the Doctrine team think that a feature is not worth having (like the Registry), then Symfony won't add it in the DoctrineBundle. And we already have quite a lot of code in the DoctrineBundle/Bridge that really belongs to Doctrine (the mapping drivers for instance).

@stof
Copy link
Member Author

stof commented Aug 19, 2011

We already have the registry. @beberlei's proposal is simply renaming it so that it is linked to the interfaces from Doctrine Common instead of the entity manager naming that is specific to the ORM. And the registry is about mainly choosing easily which entity manager (or connection, but this is less used in Symfony) to use in places where it is injected (the validator, the form type...) without having to inject the whole container and the hash of name => id. The other methods in it (getting the repository with a shortcut for instance) have been implemented for convenience after that.

Regarding the bridge, the only part that really belong to Doctrine is the Mapping subnamespace, containing the 2 drivers used to shorten the file name for XML and YAML files based on the bundle convention. All other code is about integrating Doctrine with the components.
And the code in the Mapping namespace of DoctrineBundle itself could be removed by removing the only place it is used: in the generate:entities command. Using the Doctrine implementation here would simply mean that the command would generate the entities for a single entity manager instead of all of them (just like all other commands related to Doctrine do) (and all other code in the bundle is about CLI commands, which are mainly proxies to Doctrine ones, and configuring the DIC)
So there is not so much

@fabpot
Copy link
Member

fabpot commented Aug 19, 2011

Well, I'm more than aware about all those things as I've written them all in the first place. That does not change my point of view. The Registry and the mapping drivers are temporary classes that need to find their way into the official Doctrine repositories. So, I'm not going to change/maintain/evolve the Registry if it won't be moved to Doctrine at some point.

@lsmith77
Copy link
Member

I have send a pull for DoctrinePHPCRBundle, which adds a Registry:
symfony-cmf/symfony-cmf#108

It includes a DI independent interface and implementation (should be moved to doctrine common):
https://github.com/symfony-cmf/symfony-cmf/pull/108/files#diff-1
https://github.com/symfony-cmf/symfony-cmf/pull/108/files#diff-2

Concrete implementation for PHPCR:
https://github.com/symfony-cmf/symfony-cmf/pull/108/files#diff-3

@stof
Copy link
Member Author

stof commented Oct 15, 2011

Closing this PR as the registry should be implemented using the new mecanism for Symfony 2.1

@stof stof closed this Oct 15, 2011
@lsmith77
Copy link
Member

@stof stof mentioned this pull request Oct 15, 2011
@stof stof mentioned this pull request Dec 23, 2011
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.

None yet

5 participants