FinderCollection Erroneously Returns Null & XF\Entity\Thread::getUserReadDate() Depends On This

[Jonathan M]

Affected version

2.2.5

XF\Mvc\Entity\FinderCollection extends XF\Mvc\Entity\AbstractCollection which implements ArrayAccess. This means FinderCollection should throw an exception when attempting to access non-existent keys but it just returns null instead. Since it's implementing ArrayAccess, it should behave the same way an array does when you attempt to access a key that doesn't exist.

XF\Entity\Thread::getUserReadDate() depends on this null return for the way it checks a user's read dates.

public function getUserReadDate(\XF\Entity\User $user)
{
   $threadRead = $this->Read[$user->user_id];
   $forumRead = $this->Forum ? $this->Forum->Read[$user->user_id] : null;

   $dates = [\XF::$time - $this->app()->options()->readMarkingDataLifetime * 86400];
   if ($threadRead)
   {
      $dates[] = $threadRead->thread_read_date;
   }
   if ($forumRead)
   {
      $dates[] = $forumRead->forum_read_date;
   }

   return max($dates);
}

This code assumes $threadRead will be null if the user hasn't read the thread. The same also applies to $forumRead. Once FinderCollection correctly throws an exception when attempting to access non-existent keys, this function will break.

 

[PaulB]

Thread#Read is annotated as having type \XF\Mvc\Entity\AbstractCollection|\XF\Entity\ThreadRead[], but getUserReadDate will throw an exception if Thread#Read is anything other than \XF\Mvc\Entity\FinderCollection because it assumes $this->Read[$user->user_id] will return null if the array key doesn't exist.

 

[Chris D]

This means FinderCollection should throw an exception when attempting to access non-existent keys but it just returns null instead. Since it's implementing ArrayAccess, it should behave the same way an array does when you attempt to access a key that doesn't exist.

This is an incorrect assertion, in my opinion. ArrayAccess does not need to throw an exception when accessing non-existent keys. By its nature it can behave however a developer designs it to behave. Indeed I think you'll find numerous examples in XF where this is the case and exactly the desired behaviour.

We would not be planning to make any changes to existing ArrayAccess implementations.

Thread#Read is annotated as having type \XF\Mvc\Entity\AbstractCollection|\XF\Entity\ThreadRead[], but getUserReadDate will throw an exception if Thread#Read is anything other than \XF\Mvc\Entity\FinderCollection because it assumes $this->Read[$user->user_id] will return null if the array key doesn't exist.

This may be a fair comment but there are no conceivable circumstances to my knowledge where we would expect Thread#Read to return anything other than a FinderCollection as it is accessed from the Entity getter which calls Manager::getRelation which will always return a new FinderCollection as we define a key for this specific relation.

 

[PaulB]

ArrayAccess does not need to throw an exception when accessing non-existent keys.

It would be helpful if this inconsistency between different AbstractCollection implementations were documented in AbstractCollection's PHPDoc. As it stands, ArrayCollection throws an exception while FinderCollection returns null. I would argue that this is asking for trouble, although I don't think it's reasonable to change the behavior at this point in time, since it would likely break numerous add-ons in subtle ways.

This may be a fair comment but there are no conceivable circumstances to my knowledge where we would expect Thread#Read to return anything other than a FinderCollection as it is accessed from the Entity getter which calls Manager::getRelation which will always return a new FinderCollection as we define a key for this specific relation.

That seems like it leaves an awful lot to chance. @Jonathan M ended up with the following line of code in an add-on:

$threadEntity->hydrateRelation('Read', new ArrayCollection([]));

It worked at first, and while I'm not sure I'm keen on that approach, there's nothing semantically wrong with it. Read can be any AbstractCollection, so passing in an ArrayCollection shouldn't cause a problem. Right? hydrateRelation seems to be perfectly capable of handling any AbstractCollection.

 

[Chris D]

I have left the bug report open for now so we can discuss it further but you're certainly correct that as a BC break this is significant so it's very unlikely something we'd consider changing this side of XF 3.0.

Whether we can make things clearer or otherwise safely trigger something here we'll get back to you but certainly don't expect the behaviour of the ArrayAccess implementations to significantly change at this point.

 

[Dixie McCall]

XF 3.0?