Changing private -> protected for extensibility#519
Changing private -> protected for extensibility#519benmcmath wants to merge 2 commits intojpfuentes2:masterfrom
Conversation
… the relationship instead of the table's primary key
|
Thanks, @benmcmath -- I see no reason we can't accept this now, @koenpunt ? |
|
there are probably some internal methods that will never require any override, but I'm not against. That second commit needs tests though. |
|
We likely already have sufficient coverage for that 2nd commit, right? |
|
The tests don't break by changing it, and didn't break before, so I'm pretty sure we're missing a test for the faulty behaviour. |
|
Wait, are we sure anything is faulty here? I think we have sufficient eager loading tests to capture this but perhaps not. |
| { | ||
| $this->set_keys($table->class->name); | ||
| $this->query_and_attach_related_models_eagerly($table,$models,$attributes,$includes,$this->foreign_key, $table->pk); | ||
| $this->query_and_attach_related_models_eagerly($table,$models,$attributes,$includes,$this->foreign_key, $this->primary_key); |
There was a problem hiding this comment.
@benmcmath are you able to explain the purpose this change?
There was a problem hiding this comment.
To use the primary key on the association, and not the table. Primary key can be explicitly set in the association, and that key might be different than the table's primary key. I believe if the association doesn't have a primary key, the table's primary key is used.
There was a problem hiding this comment.
Ding ding: this rings a bell now. @koenpunt is right, we'll want a test for this.
|
But if the behavior of this change was tested, it should break by now, but it doesn't, so even if this change isn't what we want, we're still missing tests for it. |
3 participants
Recently needed to extend Table.php and Model.php and it required us to convert private methods and properties to protected. Figured it would be a good change for the project.