ArrayCollection/PersistentCollection Architectural Disagreement

[WyrdNexus]

While Doctrine will provide either an ArrayCollection, or a PersistentCollection, depending on the state of the Entity, no common Class or Interface reflects their shared methods.

More specifically, when defining an entity's collection getter, the return type cannot be specified. Options include:

• PersistentCollection: in most instances, will not be correct.
• ArrayCollection: has the same methods as PersistentCollection, but in some instances will be a PersistentCollection.
• Collection/AbstractLazyCollection: in addition to the problems above, it is missing the "matching" method.

This missing common agreement complicates all code implementing these objects, which otherwise could be clean and simple.

For the sake of clarity, consider the PHP7 return type declaration on the getter for such a method. Both of the following week result in errors: pubic method getOneToMany(): ArrayCollection pubic method getOneToMany(): PersistentCollection The best alternative is: pubic method getOneToMany(): Collection ... leaving out the method: matching.

[WyrdNexus]

As a simple suggestion, perhaps Collection should implement Selectable, and the whole issue disappears.

[Majkl578]

no common Class or Interface reflects their shared methods

This is true unfortunately.

As a simple suggestion, perhaps Collection should implement Selectable, and the whole issue disappears.

Impossible due BC, would be a BC break -- only possible in next major of doctrine/collections. Also possibly not a wise idea -- consider this architecture more in general / different scenarios outside ORM. it doesn't always make sense to have selectable collection.

Regarding ORM, this could be only solved in 3.x somehow, if you have any proposals, ideally without breaking doctrine/collections, you're welcome to send PR for that or discuss here. 👍

[WyrdNexus]

Notation Key:

• () interface
• [] abscract
• definition or class
  • this extends defintion

Currently

• (Countable), (IteratorAggregate), (ArrayAccess)
  • (Collection)
• (Selectable): public function matching
• [AbstractLazyCollection] (Collection)
  • PersistentCollection (Selectable)
• ArrayCollection (Collection, Selectable)

Proposed

• (Countable), (IteratorAggregate), (ArrayAccess), (Selectable): public function matching
  • (Collection)
• [AbstractLazyCollection] (Collection)
  • PersistentCollection
• ArrayCollection (Collection)

Result

As this would neither remove nor alter any methods, classes, or interface implementations, I fail to see how this would cause a BC break. The only real limitation here, is that it would require two paired PRs against the Collections repository and the ORM Repository.

[Majkl578]

I fail to see how this would cause a BC break.

In your proposal, Collection suddenly extends Selectable, thus requires implementation for matching(). This method previously did not exist in the Collection interface so any code implementing (just) Collection is now suddenly required to also implement new matching() method, which is a BC break.

[beberlei]

The Selectable interface was added a long time after Collection was finalized. A change to this interface would have been a BC break.

The solution would be that "collections" introduces a third interface SelectableCollection that extends from Collection and Selectable and to change both ArrayCollection and PersistentCollection to use that.

This must be done in two steps: 1.) Add SelectableCollection to "doctrine/collections", have ArrayCollection extend it and release a new version (minor bump enough, because its not a BC break). 2.) Bump dependency of "doctrine/orm" to new "doctrine/collections" version and change PersistentCollection.

[michsk]

@WyrdNexus did you figure out a workaround for this issue? (without dropping the returntype typehint)

[alcaeus]

The only real limitation here, is that it would require two paired PRs against the Collections repository and the ORM Repository.

Please don't forget MongoDB ODM, where PersistentCollection doesn't implement Selectable.

[stof]

@alcaeus if a new interface is created, there is no issue for the ODM, as Collection would still not require implementing Selectable.

[alcaeus]

@stof I was replying to @WyrdNexus who suggested that the Collection interface extends Selectable, with PersistentCollection implementing Collection: this would indeed also affect ODM. A new, separate interface (SelectableCollection extends Selectable, Collection) would not directly impact ODM.

[ThomasDupont]

How do you thing about toArray() Method?

I had this issue on my documents and corrected the bug with that

    /**
     * @return array
     */
    public function getMetas(): array
    {
        return $this->metas->toArray();
    }

    /**
     * @param array $meta
     *
     * @return self
     */
    public function setMetas(array $meta): self
    {
        $this->metas = $meta;

        return $this;
    }

     /**
     * @param Meta $meta
     * 
     * @return self
     */
    public function addMeta(Meta $meta): self
    {
        $this->metas[] = $meta;

        return $this;
    }

Doctrine will parse the array before insert and The document could be manipulate without The Collection type hinting

[trickreich]

Any news/plans on this? This is really frustrating when you are working with PHPStan.

[alcaeus]

If you use phpstan/phpstan-doctrine it contains an extension to pretend that the Collection interface contains a matching method. As long as you're not using MongoDB ODM, you might be safe unless you've created custom collection implementations.

The other alternative is to annotate return types using a union type (Collection&Selectable) - PHPStan will understand this, but other tools (notably PhpStorm) may not.