Generic/ConstructorName: fix minor bug

[rodrigoprimo]

Description

This PR fixes a minor bug in Generic.NamingConventions .ConstructorName when checking if a given class has a parent. The code was checking if the lowercase version of the value returned by File::findExtendedClassName() is false. The problem is that strtolower() never returns false. It always returns a string. Thus, the condition would never be evaluated as true, and the sniff would not bail at this point when a given class has no parent.

As far as I can check, this did not cause any issues to the sniff, even for invalid code, as there is not a scenario where a class method can have a T_DOUBLE_COLON token followed by a T_STRING token with an empty content. This is true even for empty strings as PHPCS includes the quotes in the content.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.

[jrfnl]

@rodrigoprimo Thanks for this PR. Good catch!

I only have a little feedback on the analysis (but the conclusion is still the same):

This is true even for empty strings as PHPCS includes the quotes in the content.

I'm not sure I understand what you are trying to say here ?
The content of T_STRING tokens will not include quotes, but AFAIK, the content of a T_STRING token can never be empty, as in that case PHP wouldn't tokenize it as T_STRING.

So while this is a "sniff code" bug, it is not a bug which would have affected userland.

I've also marked this change now as a performance improvement as it allows the sniff to bow out earlier and do less work.

Looking at the code I do see a completely different bug, but that's outside the scope of this PR. ;-)

[jrfnl]

Looking at the code I do see a completely different bug, but that's outside the scope of this PR. ;-)

I've opened PR #652 to address the other bug(s) I saw.

[rodrigoprimo]

I'm not sure I understand what you are trying to say here ?
The content of T_STRING tokens will not include quotes, but AFAIK, the content of a T_STRING token can never be empty, as in that case PHP wouldn't tokenize it as T_STRING.

I should have provided a code sample to illustrate my point as I was trying to check if it was possible for code to trigger a false positive for this sniff, and I crafted the following invalid code:

class ClassName {
    public function __construct() {
    }

    public function ClassName() {
        ClassName::''();
    }
}

In this particular case, the '' are tokenized by PHPCS as T_STRING, and the content is not an empty string, but instead the two single quotes. That is what I was referring to.

[jrfnl]

@rodrigoprimo I see what you mean, but there are limits to the amount of fantasy code PHPCS should support ;-)