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

Add config option for model owner key #276

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wizston
Copy link

@wizston wizston commented May 12, 2017

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
Copy link

coveralls commented May 12, 2017

Coverage Status

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

Copy link
Collaborator

@Gummibeer Gummibeer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this be done by the laravel internal methods!?

(new notifynder_config()->getNotifiedModel())->getKeyName()

@wizston
Copy link
Author

wizston commented May 13, 2017

@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
Copy link
Collaborator

@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
Copy link
Author

wizston commented May 13, 2017

@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
Copy link
Collaborator

Gummibeer commented May 13, 2017

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
Copy link
Author

wizston commented May 14, 2017

Thanks @Gummibeer, I just adjusted the commit.

@coveralls
Copy link

coveralls commented May 14, 2017

Coverage Status

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

@Gummibeer
Copy link
Collaborator

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

@Gummibeer
Copy link
Collaborator

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.

3 participants