Add PHPCS scaffold tool bin/phpcs-scaffold

[ghostwriter]

Introduce a new PHPCS scaffolding utility to generate a new PHPCS sniff class, along with its unit test, fixtures and documentation files.

vendor/bin/phpcs-scaffold Standard.Category.Sniff

eg.

vendor/bin/phpcs-scaffold PHPCSDebug.Debug.TokenList

---

Will generate:

/workspace/OSS/PHPCSStandards/PHPCSDevTools/PHPCSDebug/Docs/Debug/TokenListStandard.xml
/workspace/OSS/PHPCSStandards/PHPCSDevTools/PHPCSDebug/Sniffs/Debug/TokenListSniff.php
/workspace/OSS/PHPCSStandards/PHPCSDevTools/PHPCSDebug/Tests/Debug/TokenListUnitTest.php
/workspace/OSS/PHPCSStandards/PHPCSDevTools/PHPCSDebug/Tests/Debug/TokenListUnitTest.inc.fixed
/workspace/OSS/PHPCSStandards/PHPCSDevTools/PHPCSDebug/Tests/Debug/TokenListUnitTest.inc

---

vendor/bin/phpcs-scaffold --help and bin/phpcs-scaffold -h will print.

Scaffold a new PHPCS sniff class, along with its unit test, fixtures and documentation files.

Usage:
  phpcs-scaffold Standard.Category.Sniff

Example:
  phpcs-scaffold PHPCSDebug.Debug.TokenList

Options:
  -h, --help            Print this help.

---

Todo:

• Add read me
• Add Coveralls to ci workflow
• configure the filter to the main phpunit.xml.dist and phpunitlte9.xml.dist files.

[ghostwriter]

I would love some assistance with PHPStan and PHPCS.

[jrfnl]

I would love some assistance with PHPStan and PHPCS.

What are you running into which isn't clear ? What sort of help are you looking for ?

[ghostwriter]

I would love some assistance with PHPStan and PHPCS.

What are you running into which isn't clear ? What sort of help are you looking for ?

Thanks for asking.

The PHPCS issues are clear to me, they’re mainly documentation-related (e.g., "Missing doc comment for class" and "Missing short description in doc comment"). Fixing those would mostly involve adding the required class docblocks, I'll just need to make some time after $dayJob.

For PHPStan, I’m not familiar with all the syntax for that tool and I do not plan to learn it at this time. Because of that, I’m not comfortable making changes related to its findings in a codebase that targets PHP >= 5.4.

I’d prefer not to suppress the warnings just to make CI pass, and I’m also hesitant to start changing code without knowing the preferred approach for resolving those findings here.

If someone more familiar with PHPStan could take a look at those issues, that would be helpful.

[jrfnl]

The PHPCS issues are clear to me, they’re mainly documentation-related (e.g., "Missing doc comment for class" and "Missing short description in doc comment"). Fixing those would mostly involve adding the required class docblocks, I'll just need to make some time after $dayJob.

Understood.

For PHPStan, I’m not familiar with all the syntax for that tool and I do not plan to learn it at this time. Because of that, I’m not comfortable making changes related to its findings in a codebase that targets PHP >= 5.4.

I’d prefer not to suppress the warnings just to make CI pass, and I’m also hesitant to start changing code without knowing the preferred approach for resolving those findings here.

If someone more familiar with PHPStan could take a look at those issues, that would be helpful.

Expanding/fixing up the docs should already fix some of the PHPStan issues too.

PHPStan bases a lot of analysis on the information found in docblocks, so if that info is not strictly correct, it will give false positives.

I've left a couple of hints inline for the issues reported by PHPStan.

Note: for type related issues in the tests where the test is verifying that the method under test does not act on an incorrect type, we should ignore the PHPStan issues. The whole point of that type of test is to verify what PHPStan also flags, but having it safeguarded by a test is definitely the better/more stable way to secure the type handling for the future.

Hope this helps a little.

[jrfnl]

Suggested change

	* @return void
	* @return int|false

👆🏻 This will remove the PHPStan error you can see for line 177 below 👇🏻

[jrfnl]

Adding a docblock documenting what $path is supposed to be (I suspect @param string $path) and that the method should @return void will fix the two PHPStan warnings for createDirectory().

Same principle for all the other methods.

And for an interface to be meaningful, I'd also prefer that the docblock includes a description of what the method is supposed to do (and probably why).

[jrfnl]

This might be a type supported by PHPStan, but is not a true type, so we don't use them in this codebase.
It is also not correct as the property does not have a default value, so will not be a non-empty-string until initialized.

If you want to avoid accounting for that in the @var type declaration, you may want to add a default value of ''.

Suggested change

	* @var non-empty-string
	* @var string

[jrfnl]

Suggested change

	* @return array
	* @return array<int|string, int>

[jrfnl]

@ghostwriter Thanks for opening this PR and for your willingness to contribute!

I haven't had a detailed look yet (and won't until you are ready for it and CI passes), but I did have an initial glance and read the PR description.

So please keep in mind, this is not a review, just some preliminary questions based on a quick glance.

• Will this also work for scaffolding sniffs for PHPCS itself ?

• Could you please explain the choice to use Namespace.Standard.Category.Sniff ?

  Sniff codes are build up like Standard.Category.Sniff, not Namespace.Standard.Category.Sniff.
  The 4-part code in PHPCS is for errorcodes: Standard.Category.Sniff.ErrorCode.

  So, this 4-part code is confusing to me and feels counter-intuitive.

  Also: sniffs must be namespaced, but the actual namespace may not match what you call "Namespace" in the CLI command, making this even more confusing.

  I think this may need a rethink.

  It may help to read the About Standards page in the Wiki about how Standard names, namespaces and package names tie together.

• Missing: this new tool needs to be added to the README.

Also note: it is not strictly necessary for the individual CLI tools to have a PHP 5.4 minimum. It is fine for those to have a PHP 7.2 minimum if that makes things easier (also see discussion notes about this in PR #152).
It does mean that this would need to be annotated in the README and that the bootstrap file for this particular CLI tool will need to do a PHP version check.

PR #152 and this PR would have some overlap regarding CI changes if you go down that path, so it might be interesting for you to have a look at what's in that PR.

Some other random (but more detailed) notes from that first glance:

• Please only make the necessary changes to the composer.json file and undo whatever else you did which caused the file to be "reshuffled" (making it very hard to see what you actually changed).
• Turning on Xdebug in the tests is useless when the PHPUnit config files do not have a filter configuration.
• Also: if we're going to start measuring code coverage, the workflow will need to be updated to upload coverage results to Coveralls (and the repo may need to be activated on Coveralls).
• And also: scripts should be added to the composer.json to run PHPUnit with code coverage.
• Any particular reason to add the executionOrder attribute to the PHPUnit config file ?

[ghostwriter]

@rfnl Hey Juliette,

@ghostwriter Thanks for opening this PR and for your willingness to contribute!

I haven't had a detailed look yet (and won't until you are ready for it and CI passes), but I did have an initial glance and read the PR description.

So please keep in mind, this is not a review, just some preliminary questions based on a quick glance.

I understand.

• Will this also work for scaffolding sniffs for PHPCS itself ?

No, not yet. PHPCS itself uses a different file structure. I have an idea for how to support that as well, so I’ll design a fix and update this PR accordingly.

• Could you please explain the choice to use Namespace.Standard.Category.Sniff ?
  Sniff codes are build up like Standard.Category.Sniff, not Namespace.Standard.Category.Sniff.

I was trying to be clever. Looking at the file structures used by PHPCSDevTools, PHPCSUtils, and PHPCSExtra, I noticed that each sniff class typically has a single-word namespace matching the name of a directory in the root of the project. Instead of introducing an additional CLI option, I attempted to pass the namespace as part of the sniff name using a dot-separated format.

The 4-part code in PHPCS is for errorcodes: Standard.Category.Sniff.ErrorCode.

You're right. I’m considering adding a --namespace option to explicitly configure the namespace.

So, this 4-part code is confusing to me and feels counter-intuitive.

Understood. I’ll improve this.

Also: sniffs must be namespaced, but the actual namespace may not match what you call "Namespace" in the CLI command, making this even more confusing.
I think this may need a rethink.
It may help to read the About Standards page in the Wiki about how Standard names, namespaces and package names tie together.

Thanks for the link. I’ll read it and update the PR accordingly. I wasn’t aware of that. I’ll review the wiki page and the PHP_CodeSniffer codebase to come up with a better solution.

• Missing: this new tool needs to be added to the README.

Okay. I’ll add it to the README once I have a better understanding of how to improve the CLI command and the file structure.

Also note: it is not strictly necessary for the individual CLI tools to have a PHP 5.4 minimum. It is fine for those to have a PHP 7.2 minimum if that makes things easier (also see discussion notes about this in PR #152). It does mean that this would need to be annotated in the README and that the bootstrap file for this particular CLI tool will need to do a PHP version check.

PHP 5.4 is fine for me, but I’ll review PR #152 to see if there are useful changes that could apply to this PR.

PR #152 and this PR would have some overlap regarding CI changes if you go down that path, so it might be interesting for you to have a look at what's in that PR.

I’ll take a look.

Some other random (but more detailed) notes from that first glance:

• Please only make the necessary changes to the composer.json file and undo whatever else you did which caused the file to be "reshuffled" (making it very hard to see what you actually changed).

I used composer normalize, but I will revert that and only include the necessary changes.

• Turning on Xdebug in the tests is useless when the PHPUnit config files do not have a filter configuration.

You're right. I was using a phpunit.xml file that is also in my global .gitignore ("it works on my machine"), so it wasn’t committed. I’ll add it to the repository and configure the filter to the main phpunit.xml.dist and phpunitlte9.xml.dist files.

• Also: if we're going to start measuring code coverage, the workflow will need to be updated to upload coverage results to Coveralls (and the repo may need to be activated on Coveralls).

I can add the necessary workflow changes to upload coverage results to Coveralls, for you to review. However, I don’t have access to the Coveralls account for this repository, so I won’t be able to activate it.

• And also: scripts should be added to the composer.json to run PHPUnit with code coverage.

Will do.

• Any particular reason to add the executionOrder attribute to the PHPUnit config file ?

I set executionOrder="depends,random" because it helps detect dependencies between tests.

For example, if testA and testB exist and testB only works when testA runs first, testB can explicitly declare @depends testA. PHPUnit will then ensure the correct order.

If no dependency is declared, PHPUnit will execute tests in random order, which helps detect hidden dependencies between tests.

When i was testing the Filesystem i wanted to make sure each test was independent.

---

I will commit your code suggestions, Thank you.

[jrfnl]

@ghostwriter Thanks for your response. I'll await the next iteration.

I’m considering adding a --namespace option to explicitly configure the namespace.

I'm not sure that's needed as the namespace should either be the standard name or if it diverges from that, the ruleset.xml file for a standard will have the namespace for the standard in an attribute, so you should be able to pick it up from there.

As I said before, I haven't looked at the code in detail yet, so forgive me if I'm telling you something you already figured out and/or what's already in the current code, but presuming the tool is intended to be run from the root of the project for which someone wants to scaffold a sniff, I imagine you could:

• Use a file iterator to check for ruleset.xml files up to the supported directory depth (excluding the vendor directory).
• Each ruleset.xml name will give you the name of the standard and if the namespace is not the same, the applicable namespace.
• You could then also use that info to validate whether the standard requested for the scaffolding is actually in the package.
• And determine where to place the files for that scaffolded sniff (if the package contains multiple standards).

I used composer normalize, but I will revert that and only include the necessary changes.

I'm okay with updating the file based on composer normalize in a separate PR ahead of this PR. That might help ?
The one thing I don't like about composer normalize though, is that it re-orders the scripts alphabetically. They are currently ordered by tasks, which to me is more intuitive, so when I run composer normalize, I normally undo the script re-ordering before committing the change.

However, I don’t have access to the Coveralls account for this repository, so I won’t be able to activate it.

I've turned it on for this repo, so once the config and workflow steps are in place, things should start working automatically.