-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Added support for MySQL to MongoDB relations, and vice versa for ticket ... #94
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
Conversation
I can see quite some duplicate code. Do you think it's possible to replace some code with parent::method? The more logic you delegate to the original class, the better. |
Also, the Maybe |
I chose In my own implementation I setup the alias |
The reason the code is duplicated is that until we check the |
Could be, I did not look into it that much a this moment, was just an observation :) |
To be honest, I wasn't very happy about the duplicated code and did try to limit it as much as possible, but I unfortunately couldn't find a way to do it without compromising something. I'm definitely open to changes if we can do it. |
You did not change the hasOne or hasMany relations right? Shouldn't those be modified too? |
Actually, I think I found a better way to check the relation type: if (!is_subclass_of($related, 'Jenssegers\Mongodb\Model'))
{
return parent::relation();
} |
I can't remember, but I tested all sides of it, and they all worked. I don't understand how using |
With public function belongsToMany($related, $collection = null, $foreignKey = null, $otherKey = null, $relation = null)
{
// Check if it is a relation with an original model.
if (!is_subclass_of($related, 'Jenssegers\Mongodb\Model'))
{
return parent::belongsToMany($related, $collection, $foreignKey, $otherKey, $relation);
} This way, there is no need to write any additional "duplicate" code, just check what kind of object it is and trigger the parent method. |
What about the else? The model needs to support relations all ways whether it's mongo to mongo, mysql to mysql, mysql to mongo or mongo to mysql. Since we can't extend both the Eloquent model and your model, there's no way to have access to all functions without reworking the model. |
No but this would make the Mongo Model a bit cleaner and easier. This still needs the other model for sql models. |
Would that really tidy it up though? If you think it works better than by all means. |
Check out 765e667 Could run your test with this model and see if it works? |
Testing SQL relations
All works fine on my end, well done :D |
Just thinking out loud here, what if we move the relationship logic to a intermediate class, something like a TwoWayModel, FlexModel, ModelCross or something like that. And let the Model class extend that class. Just like your ModelCross you can then use that intermediate class for 2 way relationships. Mongo model:
SQL model with Mongodb support:
The only thing the intermediate class does is check what kind of relation you are using, return the parent method for sql models and use the mongodb logic otherwise. This way, all relationship logic is grouped in 1 class without any duplicate code. |
Do you want to take a stab at that? |
I am having some problems with belongsToMany relations. That relation assumes you're working with 2 mongodb models. Or maybe I'm doing something stupid ... |
Check out f037b6a I decided to create a |
Have you tried dumping out the result of |
What do you mean? |
Well the |
No the problem with BelongsToMany is that the original relationship uses an intermediate table containing the "link". The Mongo relationship does not use an intermediate table and instead uses an attribute containing an array of id's. If it would also use an intermediate table, it would also cause problems because you would have to decide which database will "host" that table. BelongsToMany relationships will not work without quite some additional logic. |
@ollieread, are the latest changes on the develop branch working for you? I will then merge them into the master branch :) |
Let me check that. If it does, we going to close this merge request? |
Yes I will close this request :) |
Sorry I haven't had chance to test yet, going back to my hometown on Monday, so everything has been mad!! Will test asap |
I have merged the branch and updated readme: https://github.com/jenssegers/Laravel-MongoDB#mysql-relations |
Good man! |
Suggested MongoDB to MySQL cross relationships #49