Zend Naming Convention Sniff bug

squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.

BSD 3-Clause "New" or "Revised" License

10.67k stars 1.48k forks source link

Zend Naming Convention Sniff bug #810

Closed ARH-Digital closed 7 years ago

ARH-Digital commented 8 years ago

Using Zend rules. Zend/Sniffs/NamingConventions/ValidVariableNameSniff.php

Incorrectly ignores T_OBJECT_OPERATOR, example,

$Index->short_name = $Data; 135 | ERROR | Variable "short_name" is not in valid camel caps format

short_name should not be considered a variable, or it should be excluded due to the operator. This is a snake_cased value for a database column. It is not proceeded by a $ either.

gsherwood commented 8 years ago

There is no way for PHPCS to know if that member var has been set with an invalid name, or if it has been set to match a DB column name, or if it is being access using __get(). Either way, the member var name is now in the correct format and so it throws an error.

The alternative is to never check any accessed member vars. I'm not sure that it is a good change.

aik099 commented 8 years ago

In my standard I've modified the ValidVariableNameSniff to only check class property declaration, not usage. This way if the property name is wrong I get 1 error instead lots of them on every usage place.

Also with following code in checked file I also don't get false positives, because I'm not in control of XML that needs to be parsed and that surely won't be compliant with used PHPCS standard.

$xml = simplexml_load_string('<root><tag_name>test</tag_name></root>');
$tagName = $xml->tag_name;

arlaneenalra commented 8 years ago

I just ran into this.. Looking at the code I can see a few possible fixes ...

Remove this block: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/CodeSniffer/Standards/Zend/Sniffs/NamingConventions/ValidVariableNameSniff.php#L83-L112 Which would get rid of all reference time checks of member vars ..
Change the reported sniff here https://github.com/squizlabs/PHP_CodeSniffer/blob/master/CodeSniffer/Standards/Zend/Sniffs/NamingConventions/ValidVariableNameSniff.php#L104 to match the sniff here https://github.com/squizlabs/PHP_CodeSniffer/blob/master/CodeSniffer/Standards/Zend/Sniffs/NamingConventions/ValidVariableNameSniff.php#L185 This should probably also be done on the contains Numbers sniffs and would make this a maskable sniff while maintaining the disallowance of snake_case for variables in the local scope. I'll probably be throwing a PR out there in a bit ...

gsherwood commented 7 years ago

Closing this as this isn't a change I intend to make to the sniff.