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

Illuminate\Database\Eloquent\Relations\Relation::morphMap should allow numeric types #12845

Closed
christian-ehmig opened this issue Mar 24, 2016 · 9 comments

Comments

@christian-ehmig
Copy link

Currently, custom polymorphic types can be specified by either an array of morph classes or by specifying a map of type strings to classes (https://laravel.com/docs/5.2/eloquent-relationships#polymorphic-relations). However, it is not possible to use numeric keys as type "strings".

Given the following array
Relation::morphMap([ "1" => \App\Models\ModelOne::class, "2" => \App\Models\ModelTwo::class, ]);
The resulting morphMap is:
array:2 [ 0 => "App\Models\ModelOne" 1 => "App\Models\ModelTwo" ]

It should be possible to specify numeric ids which is common in database environments and also less storage hungry in terms of INT vs. VARCHARS.

@phroggyy
Copy link
Contributor

@christian-ehmig this should definitely be possible as long as you don't start at 0. That said, it's very unclear and I wouldn't personally recommend it (no one looking at that database will have a clue what references what). That said, I'll look into this.

@christian-ehmig
Copy link
Author

@phroggyy thanks. You are right by simply "looking" at the database, but you could setup the morph map types in an additional table using foreign keys for the numeric keys. Using strings within keys is no perfect idea from a database performance and storage requirement point of view.

@woodj22
Copy link

woodj22 commented Mar 8, 2017

This is a good feature, However, what happens when you have two models in two separate polymorphic related tables that are both mapped too the number one ?

How can you define the Relation::morphMap twice ?

@isometriq
Copy link

isometriq commented Jul 28, 2017

Indeed, can a morphMap set be defined per morph "group" ?

By group I mean Commentable, Taggable, Taskable, etc
If these three use their own type identifiers (zero based integer) map, what happens with collisions?

At a glance, it seems that the morph concept is geared towards unique and absolute class paths and the morphMap is only to map a unique classname to a unique identifier.

EDIT: So at the moment, I can't define this:

modules [1=>Posts, 2=>Users]
comment.related_type : [1=>'App\Post', 2=>'App\User']

sections [1=>Users, 2=>Departments]
task.related_type : [1=>'App\User', 2=>'App\Department']

..so for this, would need to be able to define separate morphMaps and decide which one to use when defining a morph* relation.

@isometriq
Copy link

isometriq commented Jul 29, 2017

I appears that one way to do it would be to override the getMorphClass eloquent model method in my app models and do whatever custom mapping i need.

Would that be a bad idea ?

@phroggyy
Copy link
Contributor

@isometriq @woodj22 can you provide a use case for this? I don't quite see how it's better to define one morph map per relation group. You end up making the database say nothing about your relations, which I'm a bit iffy about. Nonetheless, I'd like to see a good use case for this, and an argument for why it's better than a single map

@isometriq
Copy link

isometriq commented Jul 30, 2017

@phroggyy Thanks for your feedback. I agree with your concern and I'm not sure if my design has a flaw, here is an example... Use case would to treat each morphMap as a separate context and to reuse a foreign key to be also the type.

First morphMap would be for Module.
Lets say 2 modules (id:1,name:users, id:2,name:content)
Comments can be created in those two modules
Comments have a module_id int (the morph type in the morphmap) and module_related_id
if module_id is 1, morphClass would be User, if it is 2, morphClass would be Article

Second morphMap would be for DocumentType.
Lets say 2 document types (id:1,name:article, id:2,name:invoice)
Attachments can be created in those two document types
Attachments have a document_type_id int (the morph type in the morphmap) and document_id
if document_type_id is 1, morphClass would be Article, if it is 2, morphClass would be Invoice

So the idea behind this contraption is to:

  1. Avoid strings as the type and use the existing relation id (foreign key) as the actual type ..let's say DRY
  2. Allow to map a class more than once to satisfy different context associations
  3. Trying to avoid a monolithic and code-bound mapping (not a strong argument)

Example of mapping

[
    'modules' => [
        1=>'App\User',
        2=>'App\Article',
    ],
    'document_types' => [
        1=>'App\Article',
        2=>'App\Invoice',
    ],
]

Currently, could it be possible by using strings with prefixes?
(but would require a separate field for type and use it as morph type)

[
    'modules_1'=>'App\User', // or 'module_users' ..you see the picture
    'modules_2'=>'App\Article',
    'document_types_1'=>'App\Article',
    'document_types_2'=>'App\Invoice',
]

@isometriq
Copy link

isometriq commented Jul 30, 2017

The morphMap feature may allow mapping the classes so that the code classname is not in the database, which is good.

I understand that this monolithic mapping wants to associate database objects explicitly. In that case, I think the default (or even always) should not be the classname, but the related database table name (automatically). Because the best way to avoid name collisions mapping and also because Model are always bound to a database table name. Perhaps two models can use the same database table, but it doesn't matter since morph is all about data assocations in the database and not logic.

That's why I thought morphMap could be used to make custom contextual associations.

@woodj22
Copy link

woodj22 commented Jul 31, 2017

I like @isometriq explanation.

The reason I raised the point was for database organisational reasons.

For example,

I have a polymorphic relation in one part of the project that records exceptions

[
1 => 'RandomException'
2 => 'NullException
]

This mapping is then recorded in a table so that it is more accessible and more explicit than just having a key-value map in a service provider.

Now, in another part of the project I also have another polymorphic relation that has nothing to do with exceptions but i still want to use integer mappings and store them in a separate table to the exceptions.
I want the morph map to look like this

[
1=> 'User'
2 => Group
]

and then store these values in a separate table but have the same mappings.
This is not currently possible with the single group of morph mapping.

However, with all this said, I am not sure how this could be implemented. I also found that apart from the small inconvenience described above, their is no real benefit in using two separate maps and may even become even more messy. like @phroggyy described.

Instead I just used strings which are more explicit anyway and then added a 'type' field to the separate mapping table rather than having two separate tables.

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

4 participants