PHPMD ruleset is invalid

magento / magento2

Prior to making any Submission(s), you must sign an Adobe Contributor License Agreement, available here at: https://opensource.adobe.com/cla.html. All Submissions you make to Adobe Inc. and its affiliates, assigns and subsidiaries (collectively “Adobe”) are subject to the terms of the Adobe Contributor License Agreement.

http://www.magento.com

Open Software License 3.0

11.55k stars 9.32k forks source link

PHPMD ruleset is invalid #13400

Closed AntonEvers closed 4 years ago

AntonEvers commented 6 years ago

Preconditions

2.2-develop

Steps to reproduce

I want to add the ruleset dev/tests/static/testsuite/Magento/Test/Php/_files/phpmd/ruleset.xml as PhpStorm inspection

Expected result

I expect to be able to add the ruleset as seen in the bottom right of the screen shot.

Actual result

After selecting the path to dev/tests/static/testsuite/Magento/Test/Php/_files/phpmd/ruleset.xml the Custom rule sets window remains empty as in the following screen shot:

The cause of this is commit https://github.com/magento/magento2/commit/eb31539377b99d6b8f20ffd09fe1c2158398ad1b which adds the invalid node <php-includepath> to ruleset.xml. It is not supported by the current XSD. @vkublytskyi probably saw it in https://github.com/phpmd/phpmd/blob/master/src/test/resources/files/rulesets/ruleset-refs.xml which has different XSD's.

I've created an issue in PHPMD as well, because the XSD mentioned does not exist anymore: https://github.com/phpmd/phpmd/issues/545

vkublytskyi commented 6 years ago

@ajpevers, thank you for reporting this issue. I will consider how to resolve it after PHPMD maintainers response in phpmd/phpmd#546. If solution proposed by @addiks deployed then issue resolved for Magento as well.

I would like to preserve this separation of rulesets as it creates logically correct structure of components. But if updating XSD schema would be an issue for PHPMD I'll remove usage of <php-includepath> and refactor dev/tests/static/framework/Magento/CodeMessDetector

orlangur commented 6 years ago

As to me PHPMD is misused here, this check looks much more natural as a PHPCS sniff. Will propose a PR with such refactoring soon.

vkublytskyi commented 6 years ago

@orlangur, PHPCS checks coding style. Use or not to use final keyword is a design decision (such as "class should not have more than X dependencies") so it is checked by PHPMD. In other words, PHPMD responsible for "what code to write" and PHPCS for "how to write/format code".

orlangur commented 6 years ago

Well, not :) PHPCS is pretty good in any analysis which does not require more than one file analyzed at a time (like NumberOfChildren metrics in PHPMD).

https://github.com/object-calisthenics/phpcs-calisthenics-rules is a quick example of code design enforced by PHPCS, I believe there were some ECG rules implemented on top of it also, like "never load a model in loop".

My main concerns regarding current implementation is that PHPMD is quite unmaintained: https://github.com/phpmd/phpmd/releases and by design it has to check each node with $node->isFinal() instead of listening to a T_FINAL token.

vkublytskyi commented 6 years ago

@orlangur CyclomaticComplexity, NPathComplexity, ExcessiveMethodLength, ExitExpression, EvalExpression, GotoStatement - all these checks do not require analysis of more than one file, but they are part of PHPMD as they describe a quality of code and system design. PHPMD is widely used project so don't see a reason why Magento can't use it as well.

Usage of the final keyword is more close to CouplingBetweenObjects or DepthOfInheritance than to an incorrect place of curly bracket or wrong number of indents.

orlangur commented 6 years ago

@vkublytskyi PHPCS is pretty good at cyclomatic complexity actually https://stackoverflow.com/a/5851276/8640867 :) It can do much more than just "incorrect place of curly bracket or wrong number of indents".

PHPMD is widely used project so don't see a reason why Magento can't use it as well.

Yeah, I believe we are on same page that we should get maximum value from both tools. While phpmd is not actively developed the value it brings will remain constant and we can use more and more features introduced into phpcs, php-cs-fixer etc.

vkublytskyi commented 6 years ago

@orlangur, if you see benefits in the usage of PHPCS, of course, you are free to create PR with changes you would like to see in Magento. Just remember that our static tests suite should differentiate code style (formatting of the code) and design issues (separation of concerns). As well it is good to have an automatic fix for code formatting, but design issues should be fixed only by a developer.

orlangur commented 6 years ago

but design issues should be fixed only by a developer.

Yeah, I understand that final keyword removal should not be a part of autofixer :) Just like some PSR-2 violations are intentionally not fixed by phpcbf while they could technically (example: https://github.com/squizlabs/PHP_CodeSniffer/issues/1372)

magento-engcom-team commented 6 years ago

@ajpevers, thank you for your report. We've acknowledged the issue and added to our backlog.

m2-assistant[bot] commented 4 years ago

Hi @engcom-Bravo. Thank you for working on this issue. Looks like this issue is already verified and confirmed. But if you want to validate it one more time, please, go though the following instruction:

[ ] 1. Add/Edit Component: XXXXX label(s) to the ticket, indicating the components it may be related to.
[ ] 2. Verify that the issue is reproducible on 2.4-develop branch
Details
- Add the comment @magento give me 2.4-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.4-develop branch, please, add the label Reproduced on 2.4.x.
- If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
[ ] 3. If the issue is not relevant or is not reproducible any more, feel free to close it.

Join Magento Community Engineering Slack and ask your questions in #github channel.

magento-engcom-team commented 4 years ago

:white_check_mark: Confirmed by @engcom-Bravo Thank you for verifying the issue. Based on the provided information internal tickets MC-30142 were created

Issue Available: @engcom-Bravo, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

engcom-Hotel commented 4 years ago

Hello, @AntonEvers . I haven't able to reproduce the issue. The path of the file is there. Could you please check if you still have this issue and if yes - please provide more details about reproducing. Thank you.

m2-assistant[bot] commented 4 years ago

Hi @engcom-Hotel. Thank you for working on this issue. Looks like this issue is already verified and confirmed. But if you want to validate it one more time, please, go though the following instruction:

1. Add/Edit Component: XXXXX label(s) to the ticket, indicating the components it may be related to.
1. Verify that the issue is reproducible on 2.4-develop branch
  Details
  - Add the comment @magento give me 2.4-develop instance to deploy test instance on Magento infrastructure.
  - If the issue is reproducible on 2.4-develop branch, please, add the label Reproduced on 2.4.x.
  - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
1. If the issue is not relevant or is not reproducible any more, feel free to close it.

Join Magento Community Engineering Slack and ask your questions in #github channel.

engcom-Hotel commented 4 years ago

Hello, @AntonEvers . Closing the issue due to no activity. Feel free to reopen it if you still have it. Thank you.