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

adds a simple manager registry #242

Closed
wants to merge 1 commit into from
Closed

Conversation

schmittjoh
Copy link
Member

This registry adds some sane defaults and just requires a simple callable to be fully functional.

@doctrinebot
Copy link

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DCOM-160

@Ocramius
Copy link
Member

Heya! I saw this before and it uses ORM specific stuff. Can we build this on the only assumption of an ObjectManager (and related interfaces) only?

@schmittjoh
Copy link
Member Author

I guess not without extending the interface to also provide a facility for interacting with namespace aliases. This potentially requires a common config interface, not sure this is worth it.

The easier solution is to simply add support for the known manager types to this registry as I have started to do now for the entity manager. Considering that there is only a finite amount of types (maybe 5 or so), this seems to be the better solution.

@Ocramius
Copy link
Member

@schmittjoh what if we use the class metadata factory to handle ->getEntityNamespace($alias); ? It's already in all the implementations

@schmittjoh
Copy link
Member Author

That should be possible yes.

Even if this only works for the ORM right now, I think we could merge this regardless as it is a good enhancement already, and someone else can then expand it to work for other manager types either by adding them directly to the registry, or through expanding the common interfaces like you suggested.

However, I'll leave that to someone else to change as it requires changes in multiple libraries for which I don't have the time currently.

@Ocramius
Copy link
Member

@schmittjoh you can push it to ORM if it is ORM specific, or to the library that needs it if it is a workaround (as it currently is)

So please don't merge this here. We can eventually think about it in the issue I've linked before, and it will probably happen in common, but this one is neither good nor common-specific.

@schmittjoh
Copy link
Member Author

I'm not sure if you are aware of it, but instanceof does not actually require the class to be present, so you can easily extend the registry class like so:

if ($manager instanceof EntityManager) {
   // retrieve alias
} else if ($manager instanceof SomeOtherManager) {
   // retrieve alias
} else {
   throw new NotYetSupported();
}

So, it would require maybe 5 lines of code to add support for another manager without breaking anything. So, I do not consider this a workaround. It's simply a different implementation of the same functionality.

@Ocramius
Copy link
Member

@schmittjoh that will require constant patching of this class. No, it's not a solution to me

@schmittjoh
Copy link
Member Author

It will require support for maybe 5 types, there aren't more, but anyway you can submit a patch which completely changes the implementation of the getAliasNamespace method when you find the time, users of this class will not notice anyway.

@Ocramius
Copy link
Member

@schmittjoh then let's merge this once that patch is in

@schmittjoh
Copy link
Member Author

Considering that such a patch does not change the behavior of this class, and will not even be noticed by users; why should we keep users waiting for this functionality?

I had this class in my own package and thought, it might be useful to others. I can of course move it back to my library, or to a dedicated composer package, but I still don't see where this strong resistance against it comes from?

I'd love to hear some more opinions on this, @beberlei, @guilhermeblanco, @asm89.

@Ocramius
Copy link
Member

@schmittjoh if it works in your package then fine for now (aka "keep it there"). I got loads of stuff to be moved to common from the modules stuff. This doesn't mean they're ready for common.

@beberlei
Copy link
Member

@schmittjoh what is the use case for this?

@schmittjoh
Copy link
Member Author

CLI processes which need access to a database. Should be useful for something like Silex as well.

@Ocramius
Copy link
Member

Closing as "won't fix"

@Ocramius Ocramius closed this Mar 25, 2015
@Ocramius Ocramius deleted the simpleManagerRegistry branch March 25, 2015 21:59
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