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

Mongodb support #46

Open
wants to merge 6 commits into
base: 3.x
Choose a base branch
from
Open

Mongodb support #46

wants to merge 6 commits into from

Conversation

vlefr
Copy link

@vlefr vlefr commented Oct 22, 2015

Modification to support MongoDB database.

@dmishh
Copy link
Collaborator

dmishh commented Oct 23, 2015

Thanks! I'll review as soon as I have some free time

@Nyholm
Copy link
Collaborator

Nyholm commented Oct 23, 2015

This looks good. Great feature. I see that you have created a separate yaml for MongoDB and the orm. That's great!

What is the purpose of the new setting class? May I suggest to use the same setting class and then move the orm/MongoDB configuration away from the annotation and in to a separate yaml?

I do also think it is a good idea to refactor the settingsManager into one BaseSettingManager and one manager for each db.

@vlefr
Copy link
Author

vlefr commented Oct 23, 2015

Hi Nyholm,
It's good idea to refactor the settingsManager with a BaseSettingsManager. However i create another setting class to match with MongoDB pattern (i.e document class). More i use an owner property instead of ownerId.

@Nyholm
Copy link
Collaborator

Nyholm commented Oct 23, 2015

May I suggest that you do create an interface for the setting. (And maybe an abstract class?)

  • Dmishh\Bundle\SettingsBundle\Model\SettingInterface
  • Dmishh\Bundle\SettingsBundle\Document\Setting
  • Dmishh\Bundle\SettingsBundle\Entity\Setting

Now you have the SettingInterface and you should typehint on it in the setting managers.

@vlefr
Copy link
Author

vlefr commented Oct 30, 2015

Ok i'll do updates asap :)

@Nyholm
Copy link
Collaborator

Nyholm commented Mar 12, 2016

Hey @qliam
Do you have any updates on this PR?

@vlefr
Copy link
Author

vlefr commented Mar 16, 2016

Hi @Nyholm ,
Sorry, i don't have enough time since my PR.

Bye

->scalarNode('db_driver')
->validate()
->ifNotInArray($supportedDrivers)
->thenInvalid('The driver %s is not supported. Please choose one of '.json_encode($supportedDrivers))
Copy link
Owner

Choose a reason for hiding this comment

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

This could be implode instead of json_encode I think? Rest of the code looks quite okay, but I've got no MongoDB experience to validate this.

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

4 participants