search

fenos / Notifynder

Easy Internal Notification management for laravel 4.* / 5.*
MIT License
432 stars 86 forks source link

Add config option for model owner key #276

Open wizston opened 7 years ago

wizston commented 7 years ago

Option to set owner key in config for one to many relationship when polymorphic is not set to true for cases where user may have different key/table structure for their model.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-3.8%) to 86.616% when pulling e8c5a1ddb35d2de6d1ee1c973efbdd99d0b87171 on wizston:model-owner-key-config into 0d536e63ec00984353dda6ec8ea3d524e42cd822 on fenos:master.

wizston commented 7 years ago

@Gummibeer That doesn't address the purpose of this PR. This PR is to provide the option to set the foreign key in your own Model.

Gummibeer commented 7 years ago

@wizston and this is what this method returns. The method will return the key name of the related model - so in general id or any other value if you have changed this. Putting this directly in the config is, for me, overhead cause it will duplicate the configuration work.

wizston commented 7 years ago

@Gummibeer How would you advice in a situation where my model uses "user_id" column name instead of "id"? The the method returns the key name not set the key name. You wouldn't advice that i change my model relationship column name to id cause of the library, while there could be someone out there that communicated with some external data with any structure. I think adding it in the config would solve lots of these issues.

Gummibeer commented 7 years ago

Ok, what we need is the primary key of the other "user" model - this is by default id but can be changed to whatever you like - on the model and in database. The primary key of the user model is returned by his getKeyName() method. So yes, this could be a case where some work would be great - but I don't see the point why to introduce a new config value if it is possible to solve it by native laravel methods!? https://laravel.com/api/5.1/Illuminate/Database/Eloquent/Model.html#method_getKeyName

wizston commented 7 years ago

Thanks @Gummibeer, I just adjusted the commit.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-3.7%) to 86.667% when pulling 8767a15d6b99a250733dde7c859e7c25fc15d664 on wizston:model-owner-key-config into 0d536e63ec00984353dda6ec8ea3d524e42cd822 on fenos:master.

Gummibeer commented 7 years ago

Can you please fix the failed checks and in best case add an unittest for it!?

Gummibeer commented 7 years ago

https://travis-ci.org/fenos/Notifynder/jobs/232078189

Needs a review