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

Check for unsafe direct __construct() calls #7022

Closed
zonuexe opened this issue Apr 12, 2022 · 10 comments
Closed

Check for unsafe direct __construct() calls #7022

zonuexe opened this issue Apr 12, 2022 · 10 comments

Comments

@zonuexe
Copy link
Contributor

zonuexe commented Apr 12, 2022

Feature request

refs php/php-src#8351, vimeo/psalm#7857, Nyholm/psr7#198

It can be pointed out in these issues that calling __construct() from outside the object not only breaks immutability, but can also cause many unexpected behaviors.

Currently @muno92 is developing a PR for this feature.

Did PHPStan help you today? Did it make you happy in any way?

Over the last few days we have exchanged some opinions about PHP and PHPStan at PHPerKaigi in Japan. (You may have seen the picture too)

@ondrejmirtes
Copy link
Member

Yes, this should be disallowed, I agree :) Yes, I've seen the pictures and they made me really happy :)

@zonuexe
Copy link
Contributor Author

zonuexe commented Apr 12, 2022

We have categorized the use cases for the __construct() call as follows:

Safe case:

  • A: Call parent::__construct() from __construct()

Unsafe case:

  • B1: Call $foo->__construct() from outside the object
  • B2: Call parent::construct() from anything other than __construct()

Whether it is safe or not is case by case, but the case where use is justified

  • C: Initialize the object after calling ReflectionClass::newInstanceWithoutConstructor()
    • May be used, for example, for metaprogramming mock libraries and DI containers

Detecting C case is a bit difficult, so it may be out of scope for this time.

@ondrejmirtes
Copy link
Member

Yes, it's difficult, I'm fine with B1 and B2 (needs two rules - for MethodCall and StaticCall AST nodes).

@zonuexe
Copy link
Contributor Author

zonuexe commented Apr 12, 2022

Which is better, creating two rules, MethodCall and StaticCall, or one rule for CallLike?

@ondrejmirtes
Copy link
Member

Definitely two separate rules for those two specific AST nodes. CallLike means a lot more different things.

@muno92
Copy link

muno92 commented Apr 12, 2022

@ondrejmirtes
Implemention is done.

Could you review it ?

@muno92
Copy link

muno92 commented Apr 12, 2022

I found method name is not correct, so fix name.
Apologize to fix code after review request.

@nsfisis
Copy link

nsfisis commented Apr 13, 2022

As a similar case, __destruct can be invoked manually. It may cause double free.
It is, however,sometimes used intentionally to release resources at a certain, deterministic point, without waiting GC.
What do you think?

@ondrejmirtes
Copy link
Member

Implemented by: phpstan/phpstan-src#1208

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants