Updated sniff to allow automatic fixing

[cjnewbs]

Summary

• phpcs allows the sniff to provide the ability to auto-fix using phpcbf. These are 2 that come up a lot for me as PhpStorm has them as an automatic comment autocomplete thing,
• Before I start on the unit test do i need to add a new one or just modify the existing one to test for the new functionality?

Sorry if I have done this the wrong way I have not contributed before 🙂

[sivaschenko]

@cjnewbs thank you for the pull request. Can you please describe use cases / tests cases for this functionality in the description

[ihor-sviziev]

@sivaschenko,
This PR adds an ability to automatically fix the failure using phpcbf command

[cjnewbs]

Totally forgot I PR'd this. I'll try summarise test/use cases and see if i can figure out how to write a test for it.

[cjnewbs]

So I have taken some time to figure out how to write "fixer tests" and it looks like \PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest already has the built in logic to handle this. All I need to do while is to duplicate ClassAndInterfacePHPDocFormattingUnitTest.1.inc (for example) into ClassAndInterfacePHPDocFormattingUnitTest.1.inc.fixed and AbstractSniffUnitTest will compare the diffs between these 2 file and the diff between the original and what the fixer generated, and if they are the same the test passes.

That has resulted in some unexpected behaviour however. Using this document as the "source of truth" document https://devdocs.magento.com/guides/v2.4/coding-standards/docblock-standard-general.html I would say that the docblock here https://github.com/magento/magento-coding-standard/blob/develop/Magento2/Tests/Commenting/ClassAndInterfacePHPDocFormattingUnitTest.1.inc#L40 is probably not in-line with the coding standards?

/**
 * GreenHandler
 * @api Do not confuse tag for faulty short description
 */
class GreenHandler
{

}

Based on the rule "Classes and interfaces should have a short description with a human-readable description of the class. If the short description adds no additional information beyond what the type name already supplies, the short description must be omitted." this would mean the text GreenHandler should be removed but the text @api Do not confuse tag for faulty short description can stay.

Is anyone able to confirm if my reasoning/understanding on this is correct? If this is the case then within this PR I will need to modify \Magento2\Helpers\Commenting\PHPDocFormattingValidator to process this rule correctly as well as \Magento2\Tests\Commenting\ClassAndInterfacePHPDocFormattingUnitTest::getWarningList to return the updated list of failures.

For the moment I have added a new test file that I will need to tweak based on the outcome of this query.

Any pointers here would be appreciated here. 🙂

[sivaschenko]

@cjnewbs I believe your understanding is correct. Here are my thoughts:

/**
 * GreenHandler
 * @api Do not confuse tag for faulty short description
 */
class GreenHandler
{

}

Ideally should be:

/**
 * Meaningful description of the class
 *
 * @api Do not confuse tag for faulty short description
 */
class GreenHandler
{

}

However, this should also be correct:

/**
 * @api Do not confuse tag for faulty short description
 */
class GreenHandler
{

}

[cjnewbs]

I also think your 2 examples are valid. I will next:

• Update the tests with the expected outcome,
• See if I can implement this in an update to \Magento2\Helpers\Commenting\PHPDocFormattingValidator,
• Re-run the full test-suite to verify.

[ihor-sviziev]

Hi @cjnewbs,
Do you have any updates about this PR?