RFC: Improve behaviour of unsaved has_one / belongs_to relations

[tractorcow]

This RFC proposes improvements and simplification to the has_one component behaviour on dataobjects, especially in relation to unsaved records.

Note: The below more or less apply to belongs_to as well.

Current state

At the moment saving of relations works this way, with the following notes

• $obj->SomeRelation()

  • Will return an object, even if SomeRelationID is 0 (unless polymorphic, when it's null)
  • User code must use ->exists() / ->isInDB() on this object; More expensive and error prone than simple falsey check.
  • Related object will not be saved on $obj->write() unless the 4th write() parameter is set from false to true.
  • However, even if $writeComponents is set to true, the ID for the foreign key between these relations isn't set.

• $obj->SomeRelationID = $obj->ID

  • Assign new component based on given ID
  • Can't assign unsaved $obj

• return $obj->SomeRelation

  • Does nothing;

• $obj->SomeRelation = $obj

  • Does nothing, would expect it to assign the new has_one

• $obj->write() doesn't save any related objects.

Proposal

• $obj->SomeRelation / $obj->SomeRelation()

  • would act as a proper getter / setter for has_one / belongs_to relations. (code into getField / setField)
  • Not return empty objects on getter if SomeRelationID is 0. Returns null instead.

• $obj->setComponent($componentName, $item)

  • Lets you set a specific component. Alias for $obj->SomeRelation = $item
  • When components are assigned, use onBeforeWrite (for has_one) / onAfterWrite (for belongs_to) hooks to lazy-assign relation IDs instead of forcing an early write. This duplicates the UnsavedRelation behaviour, but for components instead. :)
  • Provides a way to set polymorphic has_ones. At the moment you can only really assign these on the has_many end.

• $obj->write()

  • Since we no longer create dummy objects in the relation getter, we no longer need to opt-in to writing all existing relations; All components are written on write. Deprecate 4th parameter.

5.0 breaking changes

• $obj->write()
  • Remove $showDebug
  • Remove $writeComponent
  • Use write flags instead of extra parameters
  • returns $this instead of ID for chaining

Links

Related: #3832

[tractorcow]

Partly implemented with #7819 (4.1, non-breaking changes only).

[chillu]

$obj->SomeRelation = $obj

Yeah that's a big omission in our API, and probably surprises every SS dev at least once.

$obj->SomeRelation() ... Not return empty objects on getter if SomeRelationID is 0. Returns null instead.

That's a pretty big change in terms of API use. I agree that it'd be more elegant, but that'll break lots of code in subtle ways that we can't auto-upgrade. I've written hundreds of unguarded statements relying on the current behaviour over the years, e.g. if ($obj->Member()->Email) {...}

Since we no longer create dummy objects in the relation getter, we no longer need to opt-in to writing all existing relations; All components are written on write. Deprecate 4th parameter.

Many implementations will have explicitly written the relationship instead of using the $writeComponent arg (I frankly didn't even know we had that). So by auto-writing related components alongside $obj->write(), we're causing two writes on those related components now. DataObject->onBeforeWrite() gets executed regardless if any actual data changes are written, which might alter the behaviour of custom implementations. Bit of an edge case, but worth keeping in mind if we add this to a minor release.

returns $this instead of ID for chaining

Yes, please! This one is probably a bit easier to auto-detect in an upgrader - basically warn whenever a write() is used on an object and the return value is assigned to a variable.

[tractorcow]

That's a pretty big change in terms of API use. I agree that it'd be more elegant, but that'll break lots of code in subtle ways that we can't auto-upgrade. I've written hundreds of unguarded statements relying on the current behaviour over the years, e.g. if ($obj->Member()->Email) {...}

I would probably see this as a case of errors silently failing to check assertions (e.g. checking email matching for a member that doesn't exist). As a developer, I would prefer the ORM failed early rather than silently allow possible errors to exist.

The biggest problem I have is that these "new" blank records ambiguously need to persist; The fact that you have to specify the $writeComponents arg on write() is a poor solution to this; My preference is that if a component isn't intended to be written (never explicitly assigned) it should never be created.

Big change, yes, hence a 5.0 major breaking change. :)

Many implementations will have explicitly written the relationship instead of using the $writeComponent arg (I frankly didn't even know we had that).

The important thing is that we can lazy-write the "wiring" up of the relations, not so much the data on the record themselves; The users will write these records up front because you HAVE to explicitly write them. Having auto-wiring frees the developers from having to manage write-order dependencies of trees of data.

So by auto-writing related components alongside $obj->write(), we're causing two writes on those related components now. DataObject->onBeforeWrite() gets executed regardless if any actual data changes are written, which might alter the behaviour of custom implementations.

We can make those writes conditional; I.e. ONLY write if records are changed, or if wiring is necessary but un-assigned. :P The idea is that we only ever need one write for each object, and the ORM will "magic" the order for these trees automatically to ensure this.

[kinglozzer]

$obj->SomeRelation() ... Not return empty objects on getter if SomeRelationID is 0. Returns null instead.

Definitely in favour of this change. It’s frankly bizarre behaviour which newcomers to SilverStripe don’t expect (I’ve had to explain to people before why if ($obj = $this->Relation()) { doesn‘t work as they’d expect).

[chillu]

I'm also in favour of that API, but how much upgrade disruption is it worth? You'll have to go through every line of your project code that queries a has_one relationship, and check if they're guarded correctly. If you forget one, you'll likely deal with fatal errors and quite impactful logic failures. I'd only feel comfortable doing that if we can auto-parse all uses of has_one relationships and at least list their source code locations for easy review in silverstripe/upgrader.

[tractorcow]

I'm also in favour of that API, but how much upgrade disruption is it worth? You'll have to go through every line of your project code that queries a has_one relationship, and check if they're guarded correctly. If you forget one, you'll likely deal with fatal errors and quite impactful logic failures.

I think most examples would be safe. if ($parent = $this->Parent() && $parent->exists()) would go from necessary to unnecessary, but safe.

Of course if you were just doing if ($this->Parent()->exists()) then yeah that could break, but that's the super minority of situations isn't it? has_one already has code paths that return null so existing code SHOULD be guarding it anyway.

[chillu]

I wouldn't assume that everyone is as strict on code use as you are Damian ;) Once you're used to the idioms and peculiarities of an API, you're likely exploiting them if it means less code - case in point: I've done if ($this->Parent()->exists()) many many times over the years, and would expect there's a reasonable amount of SS devs who are just as lazy as myself out there.

[dhensby]

We can't be changing the has_one methods to return non-objects in the 4.x line

[tractorcow]

No that's right. It'd be 5.x only. This RFC is for master (mostly), although I've added a few things to the last 4.1 release.