Change Closure to callable

[VincentLanglet]

Close #231

[VincentLanglet]

Psalm failing because of vimeo/psalm#8435 @greg0ire
Should I add @psalm-suppress ?

[greg0ire]

We prefer centralized ignore rules. Please add the link above when adding your ignore rule.

[VincentLanglet]

We prefer centralized ignore rules. Please add the link above when adding your ignore rule.

Done

[greg0ire]

Sorry, I meant centralized in the config file directly:

collections/psalm.xml.dist

Lines 16 to 40 in d93dbc5

	<issueHandlers>
	<MixedArgument errorLevel="info" />
	<MixedArgumentTypeCoercion errorLevel="info" />
	<MixedAssignment errorLevel="info" />
	<PossiblyNullArgument>
	<errorLevel type="suppress">
	<!-- Remove when https://github.com/vimeo/psalm/pull/7759 is released -->
	<referencedFunction name="Doctrine\Common\Collections\Collection::offsetSet" />
	</errorLevel>
	</PossiblyNullArgument>
	<UnsafeGenericInstantiation>
	<errorLevel type="suppress">
	<file name="lib/Doctrine/Common/Collections/ArrayCollection.php"/>
	</errorLevel>
	</UnsafeGenericInstantiation>
	<UndefinedAttributeClass>
	<errorLevel type="suppress">
	<!-- This class is new in PHP 8.1 and Psalm does not know it yet. -->
	<referencedClass name="ReturnTypeWillChange"/>
	</errorLevel>
	</UndefinedAttributeClass>
	</issueHandlers>

If you use a baseline file, chances are the links you add are going to go away when using a command to re-generate the baseline.

[VincentLanglet]

Fixed

but IMHO psalm-suppress are the best because as soon as the version is released the psalm suppress is reported as useless, so you dont forget.

[greg0ire]

the psalm suppress is reported as useless, so you dont forget

That's great, I didn't know about this! I wonder why it does not happen when using a config file.

[greg0ire]

Can you please add an entry to the upgrade file describing how to write an implementation of Collection that is compatible with v1 and v2? Right now this is the solution I see: https://3v4l.org/5aLXQ Using just callable doesn't work, it does not seem that Closure "extends" callable: https://3v4l.org/EZVGv

[greg0ire]

A few nitpicks

[derrabus]

Is the issue we're fixing here still relevant in PHP 8.1 and first-class callable syntax? I'd rather drop support for callables other than Closures everywhere tbh.

[VincentLanglet]

Is the issue we're fixing here still relevant in PHP 8.1 and first-class callable syntax? I'd rather drop support for callables other than Closures everywhere tbh.

I don't understand.

Every closure are callable. So you're not dropping support for Closure.
First-class callable doesn't solve the issue that these method could support all the callable.

function foo(Closure $call) {}
function bar(callable $call) {}

class A
{
	public function __invoke() {}
}

foo(strlen(...)); // works
bar(strlen(...));  // works

foo(fn () => 1); // works
bar(fn () => 1); // works

foo(new A()); // don't work
bar(new A()); // works

[greg0ire]

I didn't even know about first-class callable syntax, but now I think @derrabus point is: should we encourage usage of ugly stuff like 'strlen' when our consumers have strlen(...), since 8.1 is required:

collections/composer.json

Line 36 in cec2511

"php": "^8.1",

So you're not dropping support for Closure.

@derrabus wasn't suggesting your PR did that. I think you missed the word "other" in his answer.

[VincentLanglet]

I didn't even know about first-class callable syntax, but now I think @derrabus point is: should we encourage usage of ugly stuff like 'strlen' when our consumers have strlen(...), since 8.1 is required:

collections/composer.json

Line 36 in cec2511

"php": "^8.1",

So you're not dropping support for Closure.

I don't really see the point about restricting the param by ideology if the method can support it.
The implementation doesn't need the param to specifically to be a closure and not a callable.
If foo('strlen') works correctly, there is no reason to restrict people to write foo(strlen(...)).

It's acting like callable were deprecated by php but it's not.
A php rfc should be proposed to deprecate callable before refusing support for it.

[VincentLanglet]

I didn't even know about first-class callable syntax, but now I think @derrabus point is: should we encourage usage of ugly stuff like 'strlen' when our consumers have strlen(...), since 8.1 is required

Also that doesn't solve the issue for Class with __invoke method.
Cf #117 (comment)

[greg0ire]

In that case, maybe it's better to merge this and let PHP itself deprecate 'strlen' in a future major release.

[VincentLanglet]

In that case, maybe it's better to merge this and let PHP itself deprecate 'strlen' in a future major release.

Even if it's deprecated, callable will still be needed for invokable classes, no ?

[greg0ire]

I suppose so.

[derrabus]

The callable pseudo-type is ugly, especially the string or array kind. Unlike other types, the callable type is scope-dependant: Depending on when and where you look at it, the same array or string can be callable or not. As a consequence, callable cannot be used as property type. Moreover, on a hot path, a closure will always perform better than an array callable or a string callable. Thus, widening all interfaces to callable forces implementations to either live with more edge-cases or covert to Closure immediately everywhere.

Since with the new first-class callable syntax, creating Closure objects from existing callables has become a piece of cake, I fail to see what value the proposed change would provide on PHP 8.1.

Note that we're not simply building a brand-new API here: The PR proposes to change existing and established interfaces. A downstream implementation that wants to support v1 and v2 at the same time is either forces to drop parameter types on closure parameters completely or drop support for PHP 7 and use an ugly union type. If we were to pursue that path, it should be worth it, and I believe it's not.

---

callable will still be needed for invokable classes

a_function_that_expects_a_closure($myInvokableObject(...));
a_function_that_expects_a_closure((new MyInvokableClass)(...));

---

I understand that we would improve the situation for invokable objects a little by widening to callable. But that's still too little gain for too much pain, imho.

I appreciate the effort you've put in this PR and I'm honestly sorry that I've jumped onto this train that late. But I'm genuinely not convinced that this change is still a good idea.

[derrabus]

Examples for fun times with callables:

• https://3v4l.org/1990k
• https://3v4l.org/cH1Ek

[VincentLanglet]

I appreciate the effort you've put in this PR and I'm honestly sorry that I've jumped onto this train that late. But I'm genuinely not convinced that this change is still a good idea.

I still think callable should still be deprecated in php before refusing support for them.
But, it's you library, your call.

The issue were opened for 2 years, I recommend to close the issue if the change won't never be accepted.

[derrabus]

I still think callable should still be deprecated in php before refusing support for them.

All right, since you're just repeating arguments you've already made, let's agree to disagree. This debate does not lead anywhere.

The issue were opened for 2 years,

It might have been valid two years ago, but that does not mean we shouldn't reevaluate it after such a long time.

I recommend to close the issue if the change won't never be accepted.

Yes, we should do that.

WDYT @greg0ire?

[greg0ire]

Let's close this, I think @derrabus is making good point, and you're right, we should close the issue as well.