Fixed Reliable type hinting when using AbstractCollection/ArrayCollection/FinderCollection collections

Xon

Well-known member
Affected version
XF2.3 Beta 4
These collection classes result in entity property type hints of;
PHP:
/**
  * @property-read \MyAddon\Entity\Foo[]|\XF\Mvc\Entity\AbstractCollection
  */
This has edge cases where the exact return type doesn't really match and iterating can lose the proper type hint. Especially if calling methods on AbstractCollection

It would be nice if this was;
PHP:
/**
  * @property-read \XF\Mvc\Entity\AbstractCollection<\MyAddon\Entity\Foo>
  */

Thankfully via phpdocs we can do this:

PHP:
/**
 * @template T of \XF\Mvc\Entity\Entity
 * @template-extends \ArrayObject<string|int,T>
 * @template-implements \IteratorAggregate<T>
 * @template-implements \ArrayAccess<string|int,T>
 */
abstract class AbstractCollection implements \Countable, \IteratorAggregate, \ArrayAccess

This then requires patching the references to Entity to T, including some non-hinted return types to ensure everything accurately preserves type hinting. This includes when ArrayCollection or AbstractCollection is returned.
 
Last edited:
Sadly I can't figure out how to get XF's \XF::app()->finder() to return a type hint for the resulting finder, to have fetch and fetchOne to have the right type hint.

I did however figure out how to add a helper method on a concrete instance of the finder works. But this doesn't work for existing call sites to \XF::app()->finder() :(

Finder modifications that somewhat work:
PHP:
/**
 * @template T of \XF\Mvc\Entity\Entity
 * @template-implements \IteratorAggregate<T>
 */
class Finder implements \IteratorAggregate
{
...
    /**
     * @param int|null $limit
     * @param int|null $offset
     *
     * @return AbstractCollection<T>
     */
    public function fetch($limit = null, $offset = null)
...
}

The closes I've come if adjusting a finder like this:
PHP:
/**
 * @extends Finder<\SV\SignupAbuseBlocking\Entity\Log>
 */
class Log extends Finder
{
  /**
   * @return static
   */
    public static function finder(): self
    {
        return \XF::app()->finder(self::class);
    }
...
self wipes to generic argument linking the finder to the entity. The php 8.0 static return type would work, but for now type hinting works.
 
Last edited:
Adding these definitions to the finder gets Finder type hinting working as desired :D

PHP:
/**
 * @method AbstractCollection<\SV\SignupAbuseBlocking\Entity\Log> fetch($limit = null, $offset = null)
 * @method \SV\SignupAbuseBlocking\Entity\Log|null fetchOne($limit = null, $offset = null)
 * @extends Finder<\SV\SignupAbuseBlocking\Entity\Log>
 */
class Log extends Finder
With the opening post changes, this links the finder to the entity without any invasive code changes (just phpdoc changes).

The method definitions are sadly required currently as phpstorm isn't correctly flow the template parameter from the @extends Finder<\SV\SignupAbuseBlocking\Entity\Log> to the AbstractCollection<T> return type :( phpstorm doesn't support generic types in the @method definition, so the class name needs to be repeated a bunch :(

I think this will require a finder version of xf-dev:entity-class-properties to automate updating classes.
 
Last edited:
I did notice there was a holdover from some of the testing we did, that didn't get cleaned up or was missed before we submitted the patch. Not a huge issue, but basically there is a stray TKey which should be int|string instead:

PHP:
abstract class AbstractCollection implements \Countable, \IteratorAggregate, \ArrayAccess
{
...
    /**
     * @return array<TKey,T>
     */
	public function toArray()
and
PHP:
class ArrayCollection extends AbstractCollection
{
    /**
     * @param array<TKey,T> $entities
     */
	public function __construct(array $entities)
 
Yeah sorry I rushed to get this out as I was running out of time. I did mean to look into the TKey and fetchDeferrered stuff.
 
@Chris D

I can't believe I didn't notice, but fetchOne doesn't have the right argument list (and fetch lacks a type hint). Instead of:
PHP:
* @method \\XF\Mvc\Entity\AbstractCollection<$fqClass> fetch(\$limit = null, \$offset = null)
* @method $fqClass|null fetchOne(\$limit = null, \$offset = null)
it should be;
PHP:
* @method \\XF\Mvc\Entity\AbstractCollection<$fqClass> fetch(?int \$limit = null, ?int \$offset = null)
* @method $fqClass|null fetchOne(?int \$offset = null)
 
@Chris D I've been tinkering with some of the return type hints, and there are a couple edge cases which could do with some fixing:

AbstractCollection implements getIterator which needs it's type hint updated (updating phpstorm might have changed how it looks at this? idk):
PHP:
   /**
    * @return \ArrayIterator<int|string,T>
    */
public function getIterator(): \ArrayIterator
Finder::getIterator()'s return type similarly updating AbstractCollection<T>

The @template-extends \ArrayObject<string|int,T> probably should be removed since it doesn't actually extend that.

Now the annoying part: phpstorm appears to losing track of the array access return type hint if the definition is outside the current file.

ie;
PHP:
$recipient = $this->conversation->Recipients[0];
phpstorm thinks the type of $recipient is null|Entity.

But if you do this;
PHP:
/** @var AbstractCollection<ConversationRecipient> $recipients */
$recipients = $this->conversation->Recipients;
$recipient = $recipients[0];
phpstorm things the type of $recipient is mixed|null|ConversationRecipient|Entity. That it doesn't see the type-cast as redundant means something is being lost :(
 
@Xon I've already made the following changes for the next release, which remove \ArrayObject and adds the key to \IteratorAggregate.

PHP:
/**
 * @template T of \XF\Mvc\Entity\Entity
 * @implements \IteratorAggregate<string|int, T>
 * @implements \ArrayAccess<string|int, T>
 */

This eliminated some hinting issues with array access/iteration for me, but I've yet to try your last example.
 
Lemme guess, an open bug report that’s at least 3 years old? That’s usually the way this story ends.
 
Lemme guess, an open bug report that’s at least 3 years old? That’s usually the way this story ends.
Most of the generics related bugs I saw where solved or still in progress, at least from my initial searching. But I'm not going to hold my breath on it being fixed.
 
Lemme guess, an open bug report that’s at least 3 years old? That’s usually the way this story ends.

Uh Oh Oops GIF by 20th Century Fox Home Entertainment
 
Back
Top Bottom