Silverstripe-framework: No ORM function to add a relation on a has_one

[mediabeastnz]

There doesn't seem to be a method using the ORM to add a relation to a has_one.

Obviously you can do it like this $data->items()->add($i) but that doesn't work in reverse.

There needs to be something like this http://api.silverstripe.org/3.1/class-HasManyList.html#_add for a has_one class.

It's nice to keep everything tidy and use the ORM where possible so this might be a nice feature.

[kinglozzer]

The way most people do this (the only way I know of) is $object->HasOneID = $item->ID, where “HasOne” is the name of the relation. We have DataObject::getComponent() for extracting one to one relations, perhaps a setComponent() method might be a nice addition? There’s not much ORM magic going on behind the scenes, though, so I’m not sure whether a new method is necessary.

[jbardnz]

It seems odd that you wouldn't have an ORM function for this instead of assigning IDs directly?

Even if it wouldn't contain much ORM magic it makes sense just to keep things consistent?
I would imagine it being somewhat helpful if the attach function could check you are attaching a model of the correct type so I can't go assigning an Image to a Page relationship etc.

[dhensby]

I agree, there should be a function for this. Code should be written so that the knowledge of the DB naming isn't required.

[patricknelson]

Chiming in here to say that we also need to account for polymorphic has_one which are 1:1, not just all regular belongs_to relations (which do not have an inverse where you can utilize HasManyList or ManyManyList APIs).

Currently in my code I'm having to assign the RelationID and RelationClass fields manually everywhere this needs to be handled and, frankly, for better IDE integration (@method docblock comments aside) it's a heck of a lot simpler for me to to be DRY'er to just write my own method for my specific use case and be done with it :smile: That tells me this needs an API to help simplify refactoring and the development process in general as your code evolves.

Edit: Clarified my first paragraph to make it more obvious that it affects polymorphic relations as well.

[patricknelson]

p.s. With the change being considered minor, does this make it a candidate for 3.x (i.e. the 3 branch) or does it need to wait for 4? Shouldn't be backward incompatible; we'll be filling a void, here.

[tractorcow]

Doesn't the API simply support $parent->Child = $childRecord? Is that a more natural syntax? @patricknelson have you not tried this for your polymorphic relations?

I actually don't recall if it does, but maybe we should make it do so?

[tractorcow]

Chiming in here to say that we also need to account for polymorphic has_one and all belongs_to relations, which are 1:1 and do not have an inverse where you can utilize HasManyList or ManyManyList APIs.

Also, polymorphic has_one supports 1-to-many through PolymorphicHasManyList, so that's not quite correct. :P

[tractorcow]

Doesn't the API simply support $parent->Child = $childRecord? Is that a more natural syntax? @patricknelson have you not tried this for your polymorphic relations?

Ah, doesn't seem that this works... that would be the easiest thing to implement though, right?

[patricknelson]

Hi @tractorcow regarding this statement:

Chiming in here to say that we also need to account for polymorphic has_one and all belongs_to relations, which are 1:1 and do not have an inverse where you can utilize HasManyList or ManyManyList APIs.

Also, polymorphic has_one supports 1-to-many through PolymorphicHasManyList, so that's not quite correct. :P

I went ahead and restructured my sentence up there a bit. My core point here is that all 1:1 relations (i.e. any that use belongs_to along side a has_one) are affected and it's important to ensure we include compatibility with polymorphic relations which adds the overhead of an extra field as a foreign key. There edited :smile:

[tractorcow]

What do you think my proposal about using a basic __set-ter for assigning has_one relations?

[patricknelson]

My immediate intuition when testing for potentially undocumented behavior was to try _that_ generic assignment first just to see if it'd work. However, while I really like the idea of using assignment, my hesitation is that (at a framework level) it may not be consistent. For example, accessing auto-magical relations in SilverStripe always takes the form of a method call, right? So in this case:

// Create relationship through assignment (safer, less likely to cause fatal errors due to
// potential of accessing a method on a non-object or, in this case, undefined relationship). 
$parent->Child = $childRecord;

// More consistent with HasMany/ManyMany relationships, but doesn't make sense to use "add"
// since this is the has_one side of the relationship.
$parent->Child()->add($childRecord);

// More intuitive method-based approach, more consistent with Laravel's approach, but
// Laravel already uses methods to define relations (which I am originally used to).
$parent->Child()->save($childRecord);

// Persist on parent? Since ChildID and potentially ChildClass fields live on $parent.
$parent->write();

Also note that I'm not opposed to implementing both the assignment and method approaches. I have some experience of my own in this area since I've actually implemented my own ORM once and modeled it after Eloquent back in 2013 (I believe). What I setup used methods to define relations in the same way and had a query object which was responsible for accessing relations and aggregating objects, etc.

[patricknelson]

Hmm... Actually, I may be opposed to implementing both styles, because it may limit future backward compatible extensibility due to some unforeseen enhancements. I think the method-based approach is the most flexible long term which is also reasonably consistent.

As long as we decided between the semantic save vs. add since we need it to be singular, as this is a has_one relation we're dealing with.

[tractorcow]

// More consistent with HasMany/ManyMany relationships, but doesn't make sense to use "add"
// since this is the has_one side of the relationship.
$parent->Child()->add($childRecord);

The problem with this is that $parent->Child() is the existing mechanism for getting the child object. The object cannot be concurrently its own record, as well as the representation of a relationship with some parent, in the same way that the RelationList classes are.

[patricknelson]

@tractorcow that's true. I think in my case once you accessed the has_one relationship, you still had to call ->first() (similar to Laravel again) in order to get an instance of that object to begin handling it. Granted, you're calling ->first() frequently, but you could also potentially do more with the relationship.

So, given the current API, we're likely only limited to the attribute assignment approach. That also suggests that the property then also needs to be available as well (at least via lazy load).

[tractorcow]

I feel we are trying to force a convention in place where it doesn't really make sense. I don't think the same convention we have for lists of objects (where a separate object represents the list vs the items within) makes the same sense to has_one relation, where there is no natural separation.

I would prefer to make the parent object the container for the "has_one relation" in this case, and have the given behaviour there. E.g.

$parent->Item = null instead of $parent->Item()->remove()
$parent->Item = $obj instead of $parent->Item()->add($obj)
$parent->Item() (or even just $parent->Item) instead of $parent->Item()->first().

[patricknelson]

Right @tractorcow I agree (see my last comment). I think at this point the part we might want to establish now is:

1. If $parent->write() is required to retain this newly associated relationship (that is one setup via attribute assignment) and
2. If we agree that it make sense to allow attribute-based access like $item = $parent->Item (similar to method-based access $item = $parent->Item()) and then have that attribute based access be magic (__get()) and lazily loaded unless specified otherwise.

See what I mean?

[kinglozzer]

... and with those comments, I’m removing the effort/easy label that I mentioned on the Google Group post 😅

[kinglozzer]

@tractorcow’s proposed solution gets a +1 from me. I think intermediary objects would be out of place - add() doesn’t make sense, and set() doesn’t match the convention anyway so probably makes it more confusing.

If $parent->write() is required to retain this newly associated relationship (that is one setup via attribute assignment)

I think this is an interesting question. HasManyList::add() and ManyManyList::add() will write the relations to the database, so if we’re vying for consistency then I suppose the setter should do the same. Is it reasonable for the “magic” approach to perform the write for you, or would that be unexpected? I’m leaning towards it being unexpected... interested to hear what other people think.

If we agree that it make sense to allow attribute-based access like $item = $parent->Item (similar to method-based access $item = $parent->Item()) and then have that attribute based access be magic (__get()) and lazily loaded unless specified otherwise.

Yes I think that makes sense. __set() could write it to the database _and_ store the whole object in-memory, __get() could either fetch it from memory if it’s set or load it from the database if not.

[patricknelson]

For me, writing on $parent->Child = $childRecord is very much unexpected. Unless you ONLY write the relation. Even then, it gets complicated since $parent could be totally new ($parent->ID == 0). I'd suggest KISS principle applies here. _set(...) magic here only applies to auto-magically defining values for [Relation]ID and [Relation]Class fields on $parent when assignment takes place (so long as this is revealed to be a valid relation when setting).

[dhensby]

I'm afraid I'm much more in favour of an intermediary HasOneObject which can have a similar API to a RelationList.

It's a relation and should be treated as such.

[patricknelson]

I'm afraid I'm much more in favour of an intermediary HasOneObject which can have a similar API to a RelationList.

That's how Laravel does it. That is:

// Actual Laravel style has-one association.
$photo->album()->associate($album)
$photo->save(); // Important.

// ... would be the inverse of the has-many...
$album->photos()->add($photo)
// No need to save/write.

... and it makes sense. However, currently the SilverStripe way would be most likely:

// SilverStripe has-one association.
$photo->Album = $album
$photo->write(); // I think this should be required if you want to persist.

// ...with the inverse being...
$album->Photos()->add($photo)
// Also no need to save/write.

And this is logical as @tractorcow pointed out since $photo->Album() would already be accessible as the method-based getter. That is the only major issue with this method-based ->associate(...) approach for has_one, since it completely changes the API by basically requiring a method like ->first() to fetch that relation first to terminate the class chain. That is, ->Album() can ONLY return either an Album instance or a HasOneList instance (hypothetical name). I think it's probably bad practice to enable the ability to, say, enable the ability to call a Foobar() method on Album like $photo->Album()->FooBar() while still allowing $photo->Album()->associate($album) (i.e. via magic __call(...)). That's some wicked black magic right there :tophat: :rabbit:

[patricknelson]

@dhensby I'm in the middle on this because I like the idea of the intermediary HasOneList style approach (a la Eloquent), however we have backward compatibility issues to worry about and it's not a major benefit (as far as I can see) to go with that. Inventing it from scratch I'd say go the intermediary relation route, however, this is where we are :smile:

[tractorcow]

For me, writing on $parent->Child = $childRecord is very much unexpected. Unless you ONLY write the relation. Even then, it gets complicated since $parent could be totally new ($parent->ID == 0).

DataObject has a mechanism for storing has_one relations for unsaved records. They are stored in the $components property, so that when they are saved the actual foreign key can be generated.

I'm not sure how well it works though, and it may be worth looking at again.

[tractorcow]

RFC up at https://github.com/silverstripe/silverstripe-framework/issues/7818

[tractorcow]

I'm trying to see if I can fit in a setComponent() into 4.1.

[tractorcow]

I've implemented a basic component setter in https://github.com/silverstripe/silverstripe-framework/pull/7819