silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
721 stars 822 forks source link

Issue with multiple parent has_many relations to the same child object. #4092

Closed patricknelson closed 9 years ago

patricknelson commented 9 years ago

Apparently, DataObject->getRemoteJoinField() has a flaw in it preventing you from having two separate relations from a parent to the same child object. It appears this may only affect the 3.1 branch, since reviewing the newer code in the 3 branch, I think this bug may have been addressed.

For example, say you have a website with contests and you can have multiple grand prize winners but also have multiple runners up. Also, say you have these contests monthly and the same players will play. You can boil this down to the following:

<?php
class Contest extends DataObject {
    private static $has_many = [
        "GrandPrize" => "Person",
        "RunnerUp" => "Person",
    ];
}

class Person extends DataObject {
    private static $has_one = [
        "GrandPrize" => "Contest", // This key will match up with the "component" name (the key) on the parent object. This also becomes "GrandPrizeID" to reference back to the parent.
        "RunnerUp" => "Contest",
    ];
}

It turns out that in ->getRemoteJoinField(), which is responsible for automatically determining the foreign key (e.g. "GrandPrizeID" pointing back to the Contest object) uses array_flip() when it gets the names of the unique relations (on the child object, Person) pointing back to the parent object (Contest) and, in doing so, clobbers the first relation (GrandPrize) which means that when you attempt to get the relation for ->GrandPrizeWinners() it will return a HasManyList for RunnerUp instead.

Right now, it doesn't matter what the relation names are on the parent/child objects. However, due to the inability to determine which relation to use to reference back properly, e.g. that GrandPrizeID should be used for the ->GrandPrizeWinners() relation, it will matter what the relation name is, if you happen to have an edge case like mine where you need multiple has_many relations from the same parent to the same child (without using many_many, which admittedly may be a workaround for this).

My pull request #4093 will prefer the has_one relation pointing back that has a matching name in the parent has_many and, if no matching name is found, will continue with the existing functionality.

kinglozzer commented 9 years ago

@patricknelson Have you tried using dot-notation?

<?php
class Contest extends DataObject {
    private static $has_many = [
        "GrandPrizeWinners" => "Person.GrandPrize",
        "RunnersUp" => "Person.RunnerUp",
    ];
}

class Person extends DataObject {
    private static $has_one = [
        "GrandPrize" => "Contest",
        "RunnerUp" => "Contest",
    ];
}
patricknelson commented 9 years ago

Great Scott! That's magic. Yes, it does work. Is that documented? Also, it was slightly harder to track down since apparently for it to work I had to perform a dev/build and didn't realize it.

kinglozzer commented 9 years ago

It is documented (though it’s pretty hard to spot at a glance!): http://doc.silverstripe.org/en/developer_guides/model/relations/#has-many

patricknelson commented 9 years ago

And all that time. :facepalm:

I'm so used to using methods to define relations (both in my own ORM and in Laravel's Eloquent) where you are guided on additional optional syntax, not realizing that these strings encoded into private static variables have syntax that I overlooked which may (or may not) solve my problem. Anyway -- yet again, thanks for pointing me in the right direction and sorry for wasting your time! :D