Skip to content

Add rule call constructor#1208

Merged
ondrejmirtes merged 9 commits into
phpstan:1.6.xfrom
muno92:add_rule_call_constructor
Apr 17, 2022
Merged

Add rule call constructor#1208
ondrejmirtes merged 9 commits into
phpstan:1.6.xfrom
muno92:add_rule_call_constructor

Conversation

@muno92

@muno92 muno92 commented Apr 12, 2022

Copy link
Copy Markdown
Contributor

Implementation for phpstan/phpstan/issues/7022.

Notes

  • Check calling __construct more than twice in constructor is out of scope
    Because it is a bit difficult.

@zonuexe

zonuexe commented Apr 12, 2022

Copy link
Copy Markdown
Contributor

Already pointed out how to define rules
phpstan/phpstan#7022 (comment) phpstan/phpstan#7022 (comment)

@muno92

muno92 commented Apr 12, 2022

Copy link
Copy Markdown
Contributor Author

Already pointed out how to define rules phpstan/phpstan#7022 (comment) phpstan/phpstan#7022 (comment)

I'll split Rule

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
: ['__construct should not be called outside constructor.'];
: ['__construct() should not be called outside constructor.'];

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

This message is written in other lines, so I changed those.

@ondrejmirtes ondrejmirtes left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I'm gonna take over and bring it over the finish line 😊

@sasezaki

Copy link
Copy Markdown
Contributor

[IMO]
The rules in this PR implementation also determine that $self->__construct is also a violation,
eg. https://phpstan.org/r/0affd0af-b9e0-4ffe-9a69-ff7915120d19

but since the number of use cases is expected to be small, it can be ignored.

@muno92

muno92 commented Apr 13, 2022

Copy link
Copy Markdown
Contributor Author

[IMO]
The rules in this PR implementation also determine that $self->__construct is also a violation,
eg. https://phpstan.org/r/0affd0af-b9e0-4ffe-9a69-ff7915120d19
but since the number of use cases is expected to be small, it can be ignored.

I think, above case is no need of using __construct().
https://3v4l.org/v25j9

Is this wrong understanding?

@sasezaki

Copy link
Copy Markdown
Contributor

I think, above case is no need of using __construct().

Yep, it can be rewrite such of your example.
There is no problem with $self->__construct being a rule error, I said.

@muno92

muno92 commented Apr 13, 2022

Copy link
Copy Markdown
Contributor Author

Yep, it can be rewrite such of your example. There is no problem with $self->__construct being a rule error, I said.

Sorry. I misread your first comment.

@ondrejmirtes ondrejmirtes force-pushed the add_rule_call_constructor branch from 47828c4 to 6fa97ee Compare April 17, 2022 11:02
@ondrejmirtes ondrejmirtes force-pushed the add_rule_call_constructor branch from 6fa97ee to 4daa27c Compare April 17, 2022 11:03
@ondrejmirtes

Copy link
Copy Markdown
Member

I've pushed some improvements, feel free to check out the commits :)

@ondrejmirtes ondrejmirtes force-pushed the add_rule_call_constructor branch from 535a03e to 18930cc Compare April 17, 2022 11:10
@ondrejmirtes ondrejmirtes merged commit 58d7df3 into phpstan:1.6.x Apr 17, 2022
@ondrejmirtes

Copy link
Copy Markdown
Member

Thank you.

@muno92

muno92 commented Apr 17, 2022

Copy link
Copy Markdown
Contributor Author

Thank you, too.

@ondrejmirtes

Copy link
Copy Markdown
Member

FYI I've moved these rules to phpstan-strict-rules (124b30f, phpstan/phpstan-strict-rules@0c82c96) because it's a better fit there. According to some opinions, these rules are too opinionated to be part of the "mainstream" PHPStan. Thanks for understanding.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants