AC-659 Create phpcs static check for ModuleXMLTest #230
Conversation
| public function process(File $phpcsFile, $stackPtr) | ||
| { | ||
| $line = $phpcsFile->getTokens()[$stackPtr]['content']; | ||
| if (strpos(trim($line), '<module ') === false) { |
There was a problem hiding this comment.
It would be good to validate the file path as well
There was a problem hiding this comment.
That's already done in the ruleset
| return; | ||
| } | ||
|
|
||
| $xml = simplexml_load_string($phpcsFile->getTokensAsString(0, 999999)); |
There was a problem hiding this comment.
Is that possible to load all token until the token content ending with />?
Also, it would be good to handle possible exceptions for XML operations
There was a problem hiding this comment.
getTokenAsString returns the whole file, as there are no PHP tags inside.
| * | ||
| * @var string | ||
| */ | ||
| protected $warningCode = 'FoundObsoleteAttribute'; |
There was a problem hiding this comment.
It's better to use constant here
m2-community-project
bot
added
Progress: review
and removed
Progress: ready for review
labels
| /** | ||
| * Error violation code. | ||
| */ | ||
| const WARNING_CODE = 'FoundObsoleteAttribute'; |
There was a problem hiding this comment.
| const WARNING_CODE = 'FoundObsoleteAttribute'; | |
| private const WARNING_CODE = 'FoundObsoleteAttribute'; |
|
I think it might be good to take a look at the ways to verify this case using XSD schema, not necessarily with phpcs |
|
We have discussed the possible approaches and decided thatt loading XML and verifying specific incorrect nodes/attributes in it is fine. Full XSD validation is still present in dev mode in Magento. |
There was a problem hiding this comment.
@svera can you please take a look at the review comment and update the branch from develop. Thank you!
Magento2/ruleset.xml
Outdated
| @@ -223,6 +223,12 @@ | |||
| <severity>8</severity> | |||
| <type>warning</type> | |||
| </rule> | |||
| <rule ref="Magento2.Legacy.ModuleXML"> | |||
| <include-pattern>*\/module.xml$</include-pattern> | |||
| <include-pattern>*\/ModuleXMLUnitTest.xml$</include-pattern> | |||
There was a problem hiding this comment.
@svera please remove the test file form the patterns
m2-community-project
bot
added
Progress: changes requested
and removed
Progress: review
labels
|
@ihor-sviziev is it possible to have 2 test files? I don't think we need to follow the exact structure of the module.xml to test a specific case. |
|
@sivaschenko sure,
PS: TBH i would write one more static check against using multiple sections inside the single module.xml file, but that's another story ;) |
|
@ihor-sviziev could you please clarify this?
|
|
@svera, the regular module.xml file contains the only single section. Having tests with more than 1 section in the file isn't correct as it might cause some issues. https://github.com/magento/magento-coding-standard/tree/develop/Magento2/Tests/Classes |
👍 Got it, you're talking about the |
|
yes, for some reason, github removed it from my comment :) |
|
@ihor-sviziev done |
@ihor-sviziev done too |
m2-community-project
bot
added
Progress: approved
and removed
Progress: changes requested
labels
|
@magento import pr to magento-commerce/magento-coding-standard |
|
@svera the pull request successfully imported. |
This sniffer look for occurrences of obsolete attributes
activeandversioninmodule.xml, and returns a warning if it find any of those.