laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.54k stars 11.02k forks source link

[Proposal] Classmap for MorphTo #3090

Closed tomschlick closed 9 years ago

tomschlick commented 10 years ago

If there is not currently, it would be great to have a class to (int/char) map for the MorphTo database ability.

This would save a ton of space in the db as each record would not have to store the full path of a class. It would also provide portability for moving classes around (changing namepace) without having to update the database records, just a mapping.

Ideally this map might be passed as a parameter on the morphTo method or read from a config file that may look like this


<?php

return array(
    1 => '\Domain\Folder\ClassOne',
    2 => '\Domain\Folder\ClassTwo',
);
robclancy commented 10 years ago

I don't get it. Why?

tomschlick commented 10 years ago

Mainly for disk space purposes. Some class names can get quite long and if you couple that with a large table (millions of rows) you will start to push the limits for something that is essentially an ENUM.

patrickheeney commented 10 years ago

It should also be faster to query on since its now an integer.

JoostK commented 10 years ago

I dig this, I guess it may indeed be beneficial to performance and definitely storage.

GrahamCampbell commented 10 years ago

Sweet. We are targeting Laravel 4.2 i assume?

robclancy commented 10 years ago

Oh I get it now, I was very tired last night. I don't use this system because I am not going to put class names in my database. Also because I created my own way of doing it before this feature was added to Laravel. I have a key, for example "user" and then I also have actions, for example "created" along with an ID to send obviously. I also don't like the naming of things ending with 'able' with all this stuff. I want to call my columns 'relation' and 'relation_id'.

I won't use this, just my 2 cents on why though.

taylorotwell commented 10 years ago

I'm curious if this could be accomplished with a mutator on the model.

getFooableTypeAttribute and setFooableTypeAttribute. They would take class names but morph them for the internal $attributes array.

bencorlett commented 10 years ago

I like this. It also would play nice with inheritance, say you decided to subclass something down the track.

bencorlett commented 10 years ago

And Taylor's approach looks much nicer.

tomschlick commented 10 years ago

I can't believe I didn't think of the get/set attribute setup for this. I use those all the time. That also makes it easy to throw it into a trait to be reused over many models.

taylorotwell commented 10 years ago

So it worked?

tomschlick commented 10 years ago

I'm going to give it a shot now and see how it goes. Closing was a mistake but reopen if it doesnt work for some reason.

cheelahim commented 10 years ago

I did not find the way accessors/mutators could help to approach this issue. I see get_class() called explicitly in MorphOneOrMany constructor.

public function __construct(Builder $query, Model $parent, $type, $id, $localKey)
{
    $this->morphType = $type;
    $this->morphClass = get_class($parent);
    parent::__construct($query, $parent, $id, $localKey);
} 
phroggyy commented 9 years ago

This can be accomplished with mutators, but will not work if

  1. We wish to access the original type attribute (because of the accessor)
  2. We wish to eager load the parent model.

See #9407 for more details

tomschlick commented 9 years ago

Yeah. I don't think this is exactly the same thing as #9407 but you've done the legwork to show what needs to be done to allow this to happen.

Basically the goal of this one for me is to have global class map of strings/integers to classnames. Keeps the storage low, indexes fast, and makes sure you don't un-map data if you change a class name/namespace.

It looks like #9407 is to fix a bug with eager loading... unless I'm missing something

Also re-opening this since I forgot to so many moons ago. As @cheelahim said, its not possible without overriding some classes in it's current form.

phroggyy commented 9 years ago

@tomschlick well, my issue is an extension of this. In fact, as you can see, I have pretty much solved this issue by simply overwriting the MorphTo method on the Eloquent Model. I decided to use a class map there. However, it will fail for eager loading since our model isn't available before the MorphTo is instantiated.

GrahamCampbell commented 9 years ago

We don't process proposal issues, sorry.

tomschlick commented 9 years ago

What does that even mean? You don't allow discussion on potential changes anymore?

It has always been said that we should create an issue on a core feature before coding it up so we don't waste time. This one was created for that reason. It was determined that it couldn't be done with existing features as originally thought so I re-opened it to spark the discussion again.

taylorotwell commented 9 years ago

You can just leave them open Graham. Doesn't hurt. I think he means we don't typically code the proposals ourselves, we let the community make a PR.

On Mon, Jun 29, 2015 at 12:57 PM, Tom Schlick notifications@github.com wrote:

What does that even mean? You don't allow discussion on potential changes anymore?

It has always been said that we should create an issue on a core feature before coding it up so we don't waste time. This one was created for that reason. It was determined that it couldn't be done with existing features as originally thought so I re-opened it to spark the discussion again.

— Reply to this email directly or view it on GitHub https://github.com/laravel/framework/issues/3090#issuecomment-116775827.

GrahamCampbell commented 9 years ago

Sure. I think we need a "community" label or something so we can filter those issues out when we're looking through.

lukasgeiter commented 9 years ago

@GrahamCampbell A community label or such would be great!

GrahamCampbell commented 9 years ago

Further discussion can continue on the open PR. :)