#70 - Add doctrine odm support to be compatible with MongoDB

[BernardoSilva]

This Pull Request is for issue #70 It should extende Doctrine support to use driver odm to be compatible with MongoDB

[tmilos]

This is similar to #58 that @fnash wrote, but I like this one more since manager is not duplicated, though docs are missing. Could you just add some info to getting started instructions. You can do something similar to https://github.com/aerialship/SamlSPBundle/pull/58/files

[BernardoSilva]

Thanks for quick feedback @tmilos . Added documentation to getting started instructions like suggested. :+1:

[fnash]

@tmilos @BernardoSilva The manager is not duplicated. There is a manager for entities and a manager for Mongo documents. One manager will be loaded at once depending on the choosen driver. :)

[BernardoSilva]

@fnash thanks for remember about service config missing. The idea is that instead of programming to specific implementations we should try to use the interfaces since both managers implement ObjectManager . This way if you want to change driver you have to change less code as well. :)

[fnash]

@BernardoSilva @tmilos Okay so if you want to have only one StateStoreManager that implements ObjectManager interface, I suggest that you move the StateStoreManager outside the Entity/ directory and rename the protected field $entityManager to $objectManager and fix its class in the constructor to ObjectManager not EntityManager.

That way we will have

• a clean StateStoreManager at the root.
• Entity/StateStore.php to support ORM
• Document/StateStore.php to support MongoDB
• and 2 yaml driver files for the config.

I also suggest that the mongo driver being named mongodb.yml than odm.yml, following fos user bundle db_driver config.

[BernardoSilva]

I think is a good idea @fnash :+1: I created a ticket to be done after this one as refactoring, is that okay?

The changes about the field names already here

function __construct(ObjectManager $objectManager, $entityClass) {
        $this->objectManager = $objectManager;
       ...
}

Do you think is okay to merge this one to add the feature and close the issue #71 @tmilos ? And refactoring will be done for #80

[fnash]

You could do the refactoring in the same PR, it is a matter of changing the class name of the StateManager in the config. It is a symfony service.

[tmilos]

Just noticed, SSOStateStoreManager::getRepository function return type php doc has to be changed also from EntityRepository to ObjectRepository

Not sure about the BC with moving it out of Entity... Probably, if someone extended and customize it. Yet, if we keep it like this for mongo and bump a version afterwards it may cause BC for mongo. So, I would do it rather all before the v1.1.

So, I would do it all in this PR, but it really doesn't matter much. Though, not sure if there are some other open PRs that might conflict with that move.

[tmilos]

We should move to State\SSO\SSOStateStoreManager but also keep empty Entity\SSOStateStoreManager extending State\SSO\SSOStateStoreManager and marked as depricated.

[stof]

Instead of documenting the classes for the ORM and MongoDB while they are the same (they are not aware of the storage), I suggest using a single class for them. See how FOSUserBundle 2 does that for instance

[BernardoSilva]

@fnash changed driver name and config file name to match fos user bundle approach like you suggested. :+1: Thanks for notice that.

Nice idead @tmilos . I left an empty class extending SSOStateStoreManager like suggested.

[scrutinizer-notifier]

The inspection completed: 5 updated code elements

[stof]

@tmilos it is annoying to get notifications each time scrutinizer notifies about a change. What about making it use the commit status instead. Now that github displays multiple statuses, it works fine

[tmilos]

@stof Yes that's from the time when scrutinizer was overwriting travis' status. Haven't tried it so far. Will switch it now

[tmilos]

@fnash @BernardoSilva Shell we rename SSOStateStoreManager to DoctrineStateStore like @stof proposed, or it's ok like this and can be merged?

[fnash]

@tmilos I trust @stof for doctrine stuff. Renaming the class will make it more expressive/explicit. cc @BernardoSilva

[BernardoSilva]

What you guys think instead of renaming moving it to an even more explicit place? https://github.com/aerialship/SamlSPBundle/issues/81

I am happy to do this ticket but in a new PR. This one is just adding the feature, #81 would be refactoring not just this but the Orm approach.

@stof I think #81 goes more to fos userbundle approach and makes this bundle easy to extend for new drivers. like couchdb in userbundle example.

[BernardoSilva]

I agree it looks better and it makes sense. Renamed. Can you give feedback on #81 as future thing to do?

[tmilos]

Made alternative implementation in #83 with only one model class. Please review if it can go instead of this PR

[tmilos]

Implemented and merged by #83