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

Cannot Eager-Load Polymorphic Relationships from IoC #5039

Closed
JVMartin opened this issue Jul 12, 2014 · 3 comments
Closed

Cannot Eager-Load Polymorphic Relationships from IoC #5039

JVMartin opened this issue Jul 12, 2014 · 3 comments

Comments

@JVMartin
Copy link
Contributor

Here is a StackOverflow question that details the problem.

I develop packages for Laravel, so all of my models (and their relationships) are always bound and resolved from the IoC so that their bindings can be easily overwritten from any individual deployment.

This is simple for non-polymorphic relationships from either side, and from one side (the inverse side) of polymorphic relationships:

// Inside the \Angel\Core\Page model:
public function modules()
{
    return $this->hasMany(App::make('PageModule'));
}

public function menuItem()
{
    return $this->morphOne(App::make('MenuItem'), 'linkable');
}

However, the other (far more important) side of polymorphic relationships currently cannot do this:

// Inside the \Angel\Core\MenuLink model:
public function linkable()
{
    return $this->morphTo();
}

The relationship will only load properly if the fully namespaced model name is in the linkable_type column in the database. That is, linkable_type must be hard-coded to \Angel\Core\Page if the MenuLink is linking to a Page model. The behavior we would want is for simply Page to be stored there and \Angel\Core\Page to be resolved from the IoC (or \MyCustomModels\Page, or whatever the binding actually is).

This is, unfortunately, the case even if the properly bound class, such as \Angel\Core\Page, has a protected $morphClass = 'Page'; property - the class still will not be found without its full namespace being in the linkable_type column. (Perhaps this is a bug?)

To fix this for lazy-loading the morphTo relationship, one can simply change a single line from Eloquent's morphTo() function. Click here to see that change. I'm not suggesting this be implemented, I'm merely showing that it's a workaround for those of us that use the IoC.

Then, you can easily lazy-load models using the correct IoC bindings from the models that have this modified morphTo(). But it is still impossible to do it for eager-loading as far as I can see. Perhaps someone could give me some guidance?

Thanks so much in advance for any advice or guidance you might have.

@JVMartin JVMartin changed the title Cannot Eager-Load Polymorphic Relationships when Using IoC Cannot Eager-Load Polymorphic Relationships from IoC Jul 12, 2014
@atrauzzi
Copy link
Contributor

Correct me if I'm wrong, but isn't it undesirable to bind models to an IoC/DI container?

Definitely something I'd love to have cleared up.

@JVMartin
Copy link
Contributor Author

Atrauzzi, thanks for your comment. I would argue that it's quite desirable.

We've made dozens of production websites now, binding all models (and, indeed, all classes) to the IoC so that models can be changed or functionality can be added to any model on any site, from any package, without having to change how all of the models and packages that depend on it (or are related to it in any way) define their relationships. This works extremely well for the sites and the groups of developers that have used it. The models are in modules, and the modules are in packages. You have to extend and re-bind the models to the IoC when you extend their functionality so that you still get all updates when updating the package without overwriting your customized additions to each model.

For instance, say we have a Change model which represents each single change in the CMS. Every other model in the entire CMS has a relationship to this model through $this->hasMany(App::make('Change')), bear that in mind (spread across multiple, updateable packages). Any time you make a change to a Page, or a BlogPost, etc., this relationship is used. Someone might want to add extra functionality to each change (say, for instance, a comment system for each change), where that feature doesn't exist in the core CMS. If the Change model were not bound to the IoC, they'd need to change every single instance of $this->hasMany('\Angel\Core\Change'), to $this->hasMany('\MyModels\Change') in order to get the extended functionality working, as all other models in the framework have a relationship to the this model. Not cool. And furthermore, that's not even possible without either editing the /vendor folder (YUCK) or creating their own version of every. single. package. in the framework. But currently, all they have to do is bind over the Change model in the IoC with their new, customized model and everything works beautifully, since all the relationships are already defined as $this->hasMany(App::make('Change')) in all the models that depend upon it.

To be clear: Using the IoC in this way is already working fabulously for us. The only place it doesn't work is when eager-loading polymorphic relationships.

I was still able to get eager-loading polymorphism to work well with the IoC in my own way. This is how we're achieving query-optimized IoC-based eager-loading in our menu system until this bug is fixed:
Here is how we compile each MenuItem's related models into their groups so we can do a single query on each one (Page, BlogPost, Product, etc.)
This is where we resolve each group once and query it once, putting it back into the menu system in the proper order.

@atrauzzi
Copy link
Contributor

Being able to explain how you're using it doesn't necessarily justify it though. Regardless, I've had the advice told to me in the past to not put models in any DI/IoC. This mainly had to do with the lifecycle of objects being managed by an ORM (think of it like a 2nd container).

It's quite possible to create a more static object model that is extensible and leveraging it in a more clear and directed way. I do it all the time without involving the DI container.

There's also the other angle which is that it starts to take you back in the direction of global mutable state. So while, yes you can bind any type to a container, and obtain that type, it doesn't mean you aren't creating antipatterns by doing so.

If I had to hazard a guess, it would be along the lines of global mutable state, flattening your scope and losing control of your model lifecycle. Mainly because you've now turned your models into infrastructure and the DI container is now being manipulated at "runtime/request time".

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

No branches or pull requests

3 participants