UnsavedRelationList::first() / last() return false instead of null when list is empty

[kinglozzer]

Pull request: #11085

Affected Version

5.x

Description

UnsavedRelationList::first() and UnsavedRelationList::last() return false instead of null when the list is empty. This is inconsistent with DataList and co, which return null. Cause is the use of reset() and end():

silverstripe-framework/src/ORM/UnsavedRelationList.php

Lines 238 to 248 in 809f9e7

	public function first()
	{
	$item = reset($this->items);
	if (is_numeric($item)) {
	$item = DataObject::get_by_id($this->dataClass, $item);
	}
	if (!empty($this->extraFields[key($this->items)])) {
	$item->update($this->extraFields[key($this->items)]);
	}
	return $item;
	}

silverstripe-framework/src/ORM/UnsavedRelationList.php

Lines 255 to 262 in 809f9e7

	public function last()
	{
	$item = end($this->items);
	if (!empty($this->extraFields[key($this->items)])) {
	$item->update($this->extraFields[key($this->items)]);
	}
	return $item;
	}

I suppose there’s a minor BC risk to changing this, open to feedback on whether this is appropriate for a patch release.

[GuySartorelli]

I'm of the opinion that false is just outright wrong here and should have always been null, and that anyone relying on false will/should be doing so only as a falsy check where null will behave the same way (i.e. if (!$returnValue)).

If this is new as of CMS 5, then I doubt anyone else has even noticed the changed return type.

tl;dr: I'd support this being patched rather than waiting for a major release.

But we should probably get at least one more opinion. @silverstripe/core-team any other opinions?

[michalkleiner]

patch 👍

[madmatt]

+1 for patch

[kinglozzer]

Thanks all, PR raised

[GuySartorelli]

Merged. Will be automatically tagged and merged up by GitHub actions