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

Fix "php_unit_test_case_static_method_calls" CS #9314

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mvorisek
Copy link
Contributor

originally discussed in #9303 (comment)

This is non-functional change only, but I belive calling TestCase::assertXxx static methods as non-static like $this->assertXxx() should be replaced with self::assertXxx() because static methods should be simply called statically.

Later, we should require this using PHPStan across the whole project, for now, let's require this using PHP CS Fixer for (known) TestCase methods only.

@mvorisek mvorisek force-pushed the php_unit_test_case_static_method_calls branch 2 times, most recently from de267b5 to 394095d Compare January 22, 2024 09:37
@alecpl
Copy link
Member

alecpl commented Jan 25, 2024

I generally don't like this idea. Whenever I have an object instance I prefer to use it instead of a static call. E.g.

$var1 = $object->method();
$var2 = $object->staticMethod();

looks better than

$var1 = $object->method();
$var2 = SomeClass::staticMethod();

Maybe if a class has static methods only... So, probably TestCase is such a case, but everything inheriting from it should be using static methods (and properties) only too.

@mvorisek
Copy link
Contributor Author

mvorisek commented Jan 26, 2024

I would say you are used to that, but it is definitely less readable, as one call needs an instance, the other not. With ->method(), you cannot tell just by looking at the code. Also, PHP CS Fixer needs :: to be used to correctly make anonyous functions static which are optimized better as they do not need to be rebound before a call is made.

Let's land this for the PHPUnit static methods. It will be checked by CI, thus we can be sure only one coding style will be used.

@mvorisek mvorisek force-pushed the php_unit_test_case_static_method_calls branch from 394095d to 1842939 Compare February 7, 2024 11:00
@mvorisek mvorisek marked this pull request as draft February 25, 2024 10:26
@mvorisek mvorisek force-pushed the php_unit_test_case_static_method_calls branch 2 times, most recently from c4c0e30 to 646c04f Compare February 25, 2024 16:31
@mvorisek mvorisek marked this pull request as ready for review February 25, 2024 16:32
@mvorisek mvorisek force-pushed the php_unit_test_case_static_method_calls branch from 6ab08da to 4196bd8 Compare February 25, 2024 16:46
@mvorisek mvorisek marked this pull request as draft April 7, 2024 09:08
@mvorisek mvorisek force-pushed the php_unit_test_case_static_method_calls branch 3 times, most recently from 189937a to 514622d Compare April 7, 2024 10:33
@mvorisek mvorisek marked this pull request as ready for review April 7, 2024 10:34
@mvorisek
Copy link
Contributor Author

mvorisek commented Apr 7, 2024

PR is done and it fixes most of the ^Dynamic call to static method .+\. phpstan errors.

@mvorisek mvorisek force-pushed the php_unit_test_case_static_method_calls branch from 514622d to e5f5a35 Compare April 12, 2024 08:58
@mvorisek
Copy link
Contributor Author

@alecpl can this PR be merged?

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.

None yet

2 participants