Drop selectable extension for collection interface

[alcaeus]

Suggesting this for 0.11 since it might be considered a BC break. From the README:

Adds missing matching method on Doctrine\Common\Collections\Collection

The collection interface does not support the matching method, so the part of the extension that pretends matching exists, is downright dangeous IMO. Instead, intersection types should be used instead of the Collection interface, e.g. @var Collection&Selectable.

[ondrejmirtes]

Hi,
I understand this from the typesystem point of view, but let's be pragmatic here: all the collections in real-world use implement this Selectable interface, see:

• https://github.com/doctrine/collections/blob/master/lib/Doctrine/Common/Collections/ArrayCollection.php
• https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/LazyCriteriaCollection.php
• https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/PersistentCollection.php

That's why I implemented this extension, the error message is annoying because it's too pedantic for practical use.

[alcaeus]

That's why I implemented this extension, the error message is annoying because it's too pedantic for practical use.

That's a very disappointing reason: I've got a project using both ORM and MongoDB ODM and can't use this library because of the extension: the PersistentCollection implemented in ODM does not implement the Selectable interface, thus hiding errors that can very well appear when people don't pay attention.

I hope you reconsider your opinion - "the error message is annoying" should not be a reason to throw proper typing out the window. Just because most people do it wrong (and maybe the interfaces have been designed wrong years ago) doesn't mean we should have such code by default.

[ondrejmirtes]

“can't use this library” is a little bit exagarated, don’t you think? Just because it wouldn’t report a specific error doesn’t mean it’s unusable. Btw you can write a simple rule that checks for this anyway.

On Mon, 30 Jul 2018 at 07:23, Andreas ***@***.***> wrote: That's why I implemented this extension, the error message is annoying because it's too pedantic for practical use. That's a very disappointing reason: I've got a project using both ORM and MongoDB ODM and can't use this library because of the extension: the PersistentCollection implemented in ODM does not implement the Selectable interface, thus hiding errors that can very well appear when people don't pay attention. I hope you reconsider your opinion - "the error message is annoying" should not be a reason to throw proper typing out the window. Just because most people do it wrong (and maybe the interfaces have been designed wrong years ago) doesn't mean we should have such code *by default*. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#33 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGZuC-rqKSVp9eo6FRE8tFhE8JCH-kJks5uLphKgaJpZM4VkiQc> .

-- Ondřej Mirtes

[alcaeus]

Just because it wouldn’t report a specific error doesn’t mean it’s unusable.

Yes, it does: instead of finding errors, which PHPStan is great at, this extension hides errors for the sake of convenience instead of promoting proper typing of those interfaces. That's enough reason for me not to use the extension.

[ondrejmirtes]

To be honest, the problem is in Doctrine itself - it promotes calling “matching” on the Collection interface. Since some people are still avoiding intersection types (because they’re unsupported by other tolls) and unknown “matching” method was a common error, I decided to solve t with this extension.

On Mon, 30 Jul 2018 at 08:29, Andreas ***@***.***> wrote: Just because it wouldn’t report a specific error doesn’t mean it’s unusable. Yes, it does: instead of finding errors, which PHPStan is great at, this extension hides errors for the sake of convenience instead of promoting proper typing of those interfaces. That's enough reason for me not to use the extension. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#33 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGZuO3kVijBVjYcDnOUKJQebOyuZAC9ks5uLqe6gaJpZM4VkiQc> .

-- Ondřej Mirtes

[alcaeus]

it promotes calling “matching” on the Collection interface

If you could please point out where we do that, I’ll see to it that it’s updated accordingly. As long as the collection interface doesn’t contain a “matching” method we shouldn’t advocate using it without proper type hints and type checking. At the same time, this shouldn’t be default behavior in a static analyzer.

[ondrejmirtes]

The matching() method is presented as part of the Collection interface here: https://www.doctrine-project.org/projects/doctrine-collections/en/latest/index.html#matching

IMHO Selectable and Collection interfaces should just be merged, that would be the easiest path going forward.

[stof]

@ondrejmirtes but that cannot be done without a major version of doctrine/collections as that's a BC break.

[ondrejmirtes]

Solved in an easier way: ce2837a