Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace explicit __invoke call with regular call syntax #7981

Open
2 tasks done
julienfalque opened this issue May 3, 2024 · 11 comments
Open
2 tasks done

Replace explicit __invoke call with regular call syntax #7981

julienfalque opened this issue May 3, 2024 · 11 comments

Comments

@julienfalque
Copy link
Member

Feature request

-$callable->__invoke();
+$callable();

-$this->callable->__invoke();
+($this->callable)();

Link to Discussion where feature request was formed

No response

Contribution Checks

  • I have verified if this feature request was already discussed
  • I am familiar with "Feature or bug?"
@julienfalque julienfalque added status/to verify issue needs to be confirmed or analysed to continue kind/feature request labels May 3, 2024
@mvorisek
Copy link
Contributor

mvorisek commented May 3, 2024

Is this feasible, are you sure __invoke cannot be any/unknown method?

@julienfalque
Copy link
Member Author

What do you mean? As long as the __invoke() method exists, the object is callable as a function. The only case I see where this will fail is when __invoke() is not defined but __call() is. Although that seems unlikely, the fixer should be risky for that reason.

@mvorisek
Copy link
Contributor

mvorisek commented May 4, 2024

You are right - https://3v4l.org/fPaLC - 👍. I thought __invoke() to be present only on Closure.

@Wirone
Copy link
Member

Wirone commented May 6, 2024

The only case I see where this will fail is when __invoke() is not defined but __call() is. Although that seems unlikely, the fixer should be risky for that reason.

It does not matter, __call() has different intent and usage. We can safely (and in non-risky way) change $obj->__invoke(/* args */) to shorter $obj(/* args */).

@mvorisek
Copy link
Contributor

mvorisek commented May 6, 2024

The only case I see where this will fail is when __invoke() is not defined but __call() is. Although that seems unlikely, the fixer should be risky for that reason.

It does not matter, __call() has different intent and usage. We can safely (and in non-risky way) change $obj->__invoke(/* args */) to shorter $obj(/* args */).

It matters - https://3v4l.org/AYFu3, but I am neutral if it should be risky or not.

@Wirone
Copy link
Member

Wirone commented May 6, 2024

Ahhh, this way 😅. I think there are 2 possible paths:

  • make a pretty dumb and risky fixer that will just change ->__invoke() to (), leaving responsibility for verifying it to the user
  • make a context-aware, non-risky fixer that would change ->__invoke() to () only if we know the class being invoked and that class has __invoke() defined. BUT this is pretty much unachievable, because Fixer has context limited to a single file and most probably ->__invoke() is called outside of a invokable class, not inside of it. So I believe there are pretty low chances we can make it both safe and useful. We also don't support inheritance.

In general, seems like a job more for a Rector than Fixer.

@julienfalque
Copy link
Member Author

IMO the fixer is pretty simple to implement, and since the risky situation is very unlikely, I think it's fine to have it in PHP CS Fixer.

@Wirone
Copy link
Member

Wirone commented May 6, 2024

@julienfalque of course, I am not against this rule, I just think it will be risky anyway, since we just can't ensure proper context (including inheritance).

Side question: should the rule support 2-way standardisation? Or we consider using short invocation as a promoted standard?

@julienfalque
Copy link
Member Author

Fixing from ($foo)() to $foo->__invoke() is much more risky than from $foo->__invoke() to ($foo)(): (almost) all objects with __invoke() method are callable, but a lot of callables are not objects with __invoke() method. I would only implement the latter.

@Wirone Wirone removed the status/to verify issue needs to be confirmed or analysed to continue label May 7, 2024
@keradus
Copy link
Member

keradus commented May 8, 2024

@julienfalque in context of Wirone#5 (comment) , do you want to keep this ticket open ?

@julienfalque
Copy link
Member Author

@keradus Yes, the situations are not the same. We may prefer to avoid (string) $foo in case $foo type changes and silently introduces unexpected casts: it's not very type safe. However, $foo() is only compatible with callable type, so static analysis can have you covered: callable acts like an interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants