-
Notifications
You must be signed in to change notification settings - Fork 61
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
base: 3.x
Are you sure you want to change the base?
Mongodb support #46
Conversation
Thanks! I'll review as soon as I have some free time |
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. |
Hi Nyholm, |
May I suggest that you do create an interface for the setting. (And maybe an abstract class?)
Now you have the |
Ok i'll do updates asap :) |
Hey @qliam |
Hi @Nyholm , Bye |
->scalarNode('db_driver') | ||
->validate() | ||
->ifNotInArray($supportedDrivers) | ||
->thenInvalid('The driver %s is not supported. Please choose one of '.json_encode($supportedDrivers)) |
There was a problem hiding this comment.
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.
Modification to support MongoDB database.