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

[Xon]

Affected version

XF2.3 Beta 4

These collection classes result in entity property type hints of;

/**
  * @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;

/**
  * @property-read \XF\Mvc\Entity\AbstractCollection<\MyAddon\Entity\Foo>
  */

Thankfully via phpdocs we can do this:

/**
 * @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.

 

[Xon]

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:

/**
 * @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:

/**
 * @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.

 

[Xon]

Adding these definitions to the finder gets Finder type hinting working as desired

/**
 * @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.

 

[Chris D]

Thank you to you @Xon and @DragonByte Tech for contributing some code and a not insignificant amount of effort implementing this.

DragonByte Tech had already done some work on automatically generating skeleton finder definitions and with some of the additional changes mentioned here this all seems to be working nicely.

Thank you both again.

 

[Xon]

@Chris D it looks like fetchDeferred() isn't type hinted like fetch() is

 

[DragonByte Tech]

Might have been my bad for missing it from the generation script

 

[Xon]

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:

abstract class AbstractCollection implements \Countable, \IteratorAggregate, \ArrayAccess
{
...
    /**
     * @return array<TKey,T>
     */
	public function toArray()

and

class ArrayCollection extends AbstractCollection
{
    /**
     * @param array<TKey,T> $entities
     */
	public function __construct(array $entities)

 

[Chris D]

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.

 

[Xon]

It brought a smile to my face seeing every finder reference updated to using ::class

 

[Chris D]

These are all done for Beta 6.

 

[Xon]

@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:

* @method \\XF\Mvc\Entity\AbstractCollection<$fqClass> fetch(\$limit = null, \$offset = null)
* @method $fqClass|null fetchOne(\$limit = null, \$offset = null)

it should be;

* @method \\XF\Mvc\Entity\AbstractCollection<$fqClass> fetch(?int \$limit = null, ?int \$offset = null)
* @method $fqClass|null fetchOne(?int \$offset = null)

 

[Chris D]

Sorted

 

[Xon]

@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):

   /**
    * @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;

$recipient = $this->conversation->Recipients[0];

phpstorm thinks the type of $recipient is null|Entity.

But if you do this;

/** @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

 

[Jeremy P]

@Xon I've already made the following changes for the next release, which remove \ArrayObject and adds the key to \IteratorAggregate.

/**
 * @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.

 

[Xon]

There is phpstorm type handling bug around ArrayAccess.

 

[Chris D]

Lemme guess, an open bug report that’s at least 3 years old? That’s usually the way this story ends.

 

[Xon]

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.

 

[TickTackk]

Lemme guess, an open bug report that’s at least 3 years old? That’s usually the way this story ends.

Awaiting feedback Thread 'No ability to ask for shipping address for Stripe'

Aug 6, 2020

This means when the API key for an account which was created in India will not be able to accept payments from outside India as we are required to have shipping address.

Currently when attempting to make payment via Stripe users will be greeted with:

• TickTackk
• Replies: 9
• Forum: Bug reports

•