8000 Added support for MySQL to MongoDB relations, and vice versa for ticket ... by ollieread · Pull Request #94 · mongodb/laravel-mongodb · GitHub
[go: up one dir, main page]

Skip to content

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

Closed
wants to merge 3 commits into from

Conversation

ollieread
Copy link

Suggested MongoDB to MySQL cross relationships #49

@jenssegers
Copy link
Contributor

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.

@jenssegers
Copy link
Contributor

Also, the ModelCross class name, CrossModel sounds a bit better to me. But maybe there are some other possible names for this class?

Maybe SqlModel, or PdoModel?

@ollieread
Copy link
Author

I chose ModelCross because it is just the normal Eloquent model except that it provides cross database relationships. I feel that SqlModel or PdoModel would give the wrong impression as they're only to be used in the instance where you want a normal model to provide relationships to MongoDB.

In my own implementation I setup the alias EloquentCross.

@ollieread
Copy link
Author

The reason the code is duplicated is that until we check the instanceof we don't know whether or not to return a default relation. Creating an instance straight away and then passing to the parent will mean that we've essentially created two instances of the related model in the memory.

@jenssegers
Copy link
Contributor

Could be, I did not look into it that much a this moment, was just an observation :)

@ollieread
Copy link
Author

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.

@jenssegers
Copy link
Contributor

You did not change the hasOne or hasMany relations right? Shouldn't those be modified too?

@jenssegers
Copy link
Contributor

Actually, I think I found a better way to check the relation type:

if (!is_subclass_of($related, 'Jenssegers\Mongodb\Model'))
{
    return parent::relation();
}

@ollieread
Copy link
Author

I can't remember, but I tested all sides of it, and they all worked.

I don't understand how using is_subclass_of is better than instanceof? Also, what is parent::relation();? I kept it simple, adding another method just complicates things and you break away from the inheritance of default functionality.

@jenssegers
Copy link
Contributor

With is_subclass_of you don't need to create an instance of the object, so you can add something like this before every relation method: (http://www.php.net/manual/en/function.is-subclass-of.php)

    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.

@ollieread
Copy link
Author

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.

@jenssegers
Copy link
Contributor

No but this would make the Mongo Model a bit cleaner and easier. This still needs the other model for sql models.

@ollieread
Copy link
Author

Would that really tidy it up though? If you think it works better than by all means.

@jenssegers
Copy link
Contributor

Check out 765e667

Could run your test with this model and see if it works?

Ollie Read and others added 2 commits December 19, 2013 08:24
@ollieread
Copy link
Author

All works fine on my end, well done :D

@jenssegers
Copy link
Contributor

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:

Mongodb\Model -> Mongodb\TwoWayModel -> Eloquent\Model

SQL model with Mongodb support:

Mongodb\TwoWayModel -> Eloquent\Model

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.

@ollieread
Copy link
Author

Do you want to take a stab at that?

@jenssegers
Copy link
Contributor

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 ...

@jenssegers
Copy link
Contributor

Check out f037b6a

I decided to create a Jenssegers/Eloquent/Model which contains the relationship logic for both models.

@ollieread
Copy link
Author

Have you tried dumping out the result of is_subclass_of?

@jenssegers
Copy link
Contributor

What do you mean?

@ollieread
Copy link
Author

Well the belongsToMany() is assuming that you're working with 2 mongodb models, perhaps it's an instance of something else?

@jenssegers
Copy link
Contributor

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.

@jenssegers
Copy link
Contributor

@ollieread, are the latest changes on the develop branch working for you? I will then merge them into the master branch :)

@ollieread
Copy link
Author

Let me check that. If it does, we going to close this merge request?

@jenssegers
Copy link
Contributor

Yes I will close this request :)

@ollieread
Copy link
Author

Sorry I haven't had chance to test yet, going back to my hometown on Monday, so everything has been mad!! Will test asap

@jenssegers
Copy link
Contributor

I have merged the branch and updated readme: https://github.com/jenssegers/Laravel-MongoDB#mysql-relations

@jenssegers jenssegers closed this Dec 23, 2013
@ollieread
Copy link
Author

Good man!

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.

2 participants
0