should the return type of ReadableCollection:filter() & ReadableCollection:map() not be static ?

[dgoosens]

hi,

This is more a question than an issue...
Got the feeling the return types of ReadableCollection:filter() & ReadableCollection:map() should be static instead of the current ReadableCollection

This is a simplified version of what I'm doing:

class Foo
{
    private function __construct(
        public Collection $collection,
    )
    { }

    public static function of(Collection $collection): self
    {
        $collection = $collection->filter(fn($item) => $item->test());
        return new self($collection);
    }
}

And PhpStan complains about this.... because, as it is, $collection->filter() will return a ReadableCollection not a Collection, which is obviously what is to be expected IMHO.

By the way, when looking at the implementation of ArrayCollection, the defined return type is static.

But I might be completely wrong about this...
Therefor asking the question before bluntly stating this is an issue...

quite possible the return type might need a change for other methods, did not take the time to review everything...
would definitely submit a PR if the above question is indeed an issue

[derrabus]

I don't think that the interface should force implementations to return an instance of the same class which is what static would do. That being said, I also think that the return type of Collection:filter() should be Collection.

[dgoosens]

I also think that the return type of Collection:filter() should be Collection.

that's the main point really...

maybe an union type static | ReadableCollection ?
not sure if that's really a great idea

[derrabus]

I also think that the return type of Collection:filter() should be Collection.

that's the main point really...

So let's fix that. PR welcome.

maybe an union type static | ReadableCollection ?

Which is essentially the same as ReadableCollection, isn't it.

[stof]

static would be wrong. All implementations of the interface currently return an ArrayCollection for those operations, not an instance of themselves. So this would be a total lie.

[buffcode]

ArrayCollection already overrides the return type to static. Custom collections such as Doctrine\ODM\MongoDB\PersistentCollection which do not override the return type will return ReadableCollection by default, so with those classes it is currently not possible to

1. create a CustomCollection,
2. filter on it,
3. and finally add items

because the return type in step 2 is a ReadableCollection without an add method. But maybe that could also be fixed in doctrine/mongodb-odm 🤔

[stof]

Well, a PersistentCollection will definitely not return static when you filter it. The returned collection will not be a persistent one (most likely, the implementation being used will be an ArrayCollection). ReadableCollection should definitely not enforce returning the same implementation of collection, as any other implementation of Collection than ArrayCollection (be it the LazyCollection, the PersistentCollection of the ORM or the collections of doctrine/mongodb-odm) would then be unable to implement that interface.

Anything you add in that collection obtained from the filtering would not be added to the original collection (that's also true when using an ArrayCollection as the original one btw). So I fail to see what is the use case for adding elements in a collection being filtered.

[buffcode]

@stof Doctrine MongoDB Bundle uses ArrayCollection as default, but the collection class can be overridden per Document-Type, but Collection is a safe choice anyway then.

Of course those collections are not meant to replace the original persistent collections, as the filter operations always returns a new instance. But this way you can grab a persistent collection, filter it, add some items and persist them again (via an explicit operation of course).

All the code is still working technically, but breaking in type-checks with collections v2 because of ReadableCollection return type. In v1 filter was was defined on Collection itself returning Collection<TKey, T>.

Btw. the same applies to Collection::map which will also return a ReadableCollection.

Maybe it is sufficient to add the return types in the Collection class?

/**
 * ...
 * @method Collection<TKey, T> filter(Closure $p)
 * @method Collection<TKey, T> map(Closure $p)
 */
interface Collection extends ReadableCollection, ArrayAccess
{

[heiglandreas]

From what I gathered the main issue was removing the filter and set override from the Collection interface definition in 085d4db

Add that to the fact that the implementing classes (AbstractLazyCollection for example) are using

/**
 * {@inheritDoc}
 */

which then means that they are now inheriting the docs from the ReadableCollection instead of the Collection... 😕

How to solve that?

I'd say the best solution would be to explicitly declare the return type in the implementing classes and not just inherit whatever comes by. And in the declaring classes one could use static instead of the explicit class name - at least in abstract classes.

Shall I prepare a PR?

In the meantime I have solved the issue I got with PHPStan in my code like this:

 /**
  * @return Collection<array-key, SubEntity>
  */
 public function getCollection(): Collection
 {
    /** @var Collection<array-key, SubEntity> $f */
    $f = $this->subEntity->filter(function(SubEntity|null $subEntity = null) {
        if (! $subEntity instanceof SubEntity) {
            return false;
        }
         return true;
    });

    return $f;
}

I'd love to avoid this hacky temporary variable but it solves the issue.

[heiglandreas]

Actually, in the 3.x branch the implementation of AbstractLazyCollection::filter looks like this:

collections/src/AbstractLazyCollection.php

Lines 180 to 191 in bdd8b62

	/**
	* @psalm-param Closure(T, TKey):bool $p
	*
	* @return ReadableCollection<mixed>
	* @psalm-return ReadableCollection<TKey, T>
	*/
	public function filter(Closure $p): ReadableCollection
	{
	$this->initialize();
	return $this->collection->filter($p);
	}

whereas the implementation of ArrayCollection::filter looks like this:

collections/src/ArrayCollection.php

Lines 315 to 324 in bdd8b62

	/**
	* {@inheritDoc}
	*
	* @return static
	* @psalm-return static<TKey,T>
	*/
	public function filter(Closure $p): Collection
	{
	return $this->createFrom(array_filter($this->elements, $p, ARRAY_FILTER_USE_BOTH));
	}

Should their doc-block not be kinda the same?

[stof]

Well, ArrayCollection gives an extra guarantee that filtering it still returns an ArrayCollection.
But we cannot put that guarantee in the interface (filtering a LazyCollection does not return a LazyCollection)

[heiglandreas]

We could in the Collection Interface. But we can for sure in the AbstractLazyCollection as that is the concrete implementation. And there the filter-method operates on a Collection which means it will also return a Collection.

[stof]

Well, Collection could enforce returning Collection. It cannot enforce returning static

[stof]

@heiglandreas filtering an AbstractLazyCollection does not return an AbstractLazyCollection most of the time but an ArrayCollection.

[heiglandreas]

It will definitely return a Collection-implementation.

[stof]

@heiglandreas and that is not what static requires.

[heiglandreas]

I was not explicitly talking about static. AbstractLazyCollection::filter operates on a Collection. So it will at least return a Collection. So that should be the return type. Not ReadableCollection.