Modernize sniffs & helpers and embrace PHPCSUtils

[gmazzap]

Please check if the PR fulfills these requirements

• The commit message follows our guidelines
• Tests for the changes have been added (for bug fixes/features)
• Docs have been added/updated (for bug fixes/features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Bug fix, feature

What is the current behavior? (You can also link to an open issue here)

See #72 and #73

What is the new behavior (if this is a feature change)?

• Split PHPCSHelpers class in multiple smaller classes, easier to test and maintain reducing complexity via PHPCSUtils
• Make use of new helpers and PHPCSUtils in all sniffs
• Modernize sniffs and their tests for modern PHP feautures
• Re-organize and extend tests
• Standardize file headers (license & co)

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

Yes. The old helper class is deleted.

Other information:

Sorry for the big PR. The helpers where all interconnected and used in sniffs, it was hard to refactor one without touching the others.
And then I needed to test the work, so I adjusted the test.
I could have put the license normalization in another PR, but I guess that is the lease of the problems reviewing this PR...

[tfrommen]

Thanks for refactoring, improving and simplifying this, @gmazzap!

I have two questions that each may pose some massive churn, but still:

1. The Inpsyde\Helpers namespace is new, right? If yes, can we maybe choose a more specific name, for example, Inpsyde\CSHelpers, or Inpsyde\CodingStandardsHelpers or something like that? Just "Helpers" really is too generic, in my opinion.
2. Why did you decide to use that many Nowdoc blocks in tests, each representing a file-under-test? Can we move the code into actual files, maybe? That would mean that the test code is much slimmer, and one would have (better) IDE-support when writing/editing such files...

Again, thanks! I really like upgrading this to using the now-stable PHPCSUtils package.

[gmazzap]

@tfrommen

the Inpsyde\Helpers namespace is new, right? If yes, can we maybe choose a more specific name.

Agreed. For test we already have Inpsyde\CodingStandard\Tests I guess we can use Inpsyde\CodingStandard\Helpers

Edit: implemented in e3a3488.

---

Why did you decide to use that many Nowdoc blocks in tests, each representing a file-under-test?

I guess you are referring to the helpers' tests. The thing is some tests case have a different Nowdoc blocks per test method, see e.g. https://github.com/inpsyde/php-coding-standards/blob/feature/72-73/tests/cases/Helpers/FunctionDocBlocTest.php That would mean we would need different files per test case, and it would be hard to organize or even name them.

Moreover, I think that when the Nowdoc is inside a certain limit, let's say 50 lines or so, it is still "manageable" especially when the IDE can easily "collapse" those blocks. And to me readability is actually better, not worse.

Take this example, for a real test case. And imagine it would be written like this:

 public function testAllTags(): void { $file = $this->factoryFile(getenv('FIXTURES_PATH') . '/helpers/function-docbloc-001.php')); $oneFuncPos = $file->findNext(T_FUNCTION, 1); $twoFuncPos = $file->findNext(T_CLOSURE, $oneFuncPos + 1); $tagsOne = FunctionDocBlock::allTags($file, $oneFuncPos); $tagsTwo = FunctionDocBlock::allTags($file, $twoFuncPos); static::assertSame( [ '@param' => ['?string $foo', 'bool $bool'], '@return' => ['string'], ], $tagsOne ); static::assertSame( [ '@param' => ['string $foo'], '@return' => ['string'], '@custom' => ['Hello World'], '@wp-hook' => [''], ], $tagsTwo ); }

The assertions make totally no sense if you don't look at the actual PHP code that is being analyzed, which in this case would require the developer to open another file.
Or reading that same code on GitHub would require to open the two files in two different tabs and switch from one to the other.
Having a 20-lines Nowdoc in place, like we have now, IMO makes it easier to understand what the test is about, without the need to open two files or two browser tabs.

[tfrommen]

Thanks for renaming the Helpers' namespace. 🙏

Regarding the inline Nowdoc PHP code - I guess this comes down to personal preference.
If this was testing some actual production code only (and no additional config or files), you would also not inline that code into the test method.
Reading a proper unit test may result in asking questions about what is happening. And that is OK, in my opinion.

• "Why does this test ensure the get_transient function returns true twice, and false a third time?"
• "Why does this method set random data in $_GET?"

Unit tests require knowledge about the actual implementation details. Just like any other white-box test.

In this case, you can structure the test like so, because a part of the subject-under-test is some PHP code that gets analyzed, and that could live in an actual file, or in a string. So if you prefer this, that is fine by me.