Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File::isReference has problems with some bitwise operators and class property references #1604

Closed
umherirrender opened this issue Aug 11, 2017 · 4 comments · Fixed by #1609
Closed
Milestone

Comments

@umherirrender
Copy link

In the mediawiki/core code the Squiz.WhiteSpace.OperatorSpacing was added, but it reports a false positive, see [1]
| ERROR | [x] Expected 1 space after "&" operator; 0 found | | (Squiz.WhiteSpace.OperatorSpacing.NoSpaceAfterAmp) | ERROR | [x] Expected 1 space after "&" operator; 0 found | | (Squiz.WhiteSpace.OperatorSpacing.NoSpaceAfterAmp)

It is not reported, when using the long array syntax, because T_ARRAY is checked.

[1] https://gerrit.wikimedia.org/r/#/c/371474/3/includes/parser/ParserOptions.php

@WMDE-Fisch
Copy link

For the record: This happened with version 3.0.2

@jrfnl
Copy link
Contributor

jrfnl commented Aug 13, 2017

@umherirrender @WMDE-Fisch I believe PR #1609 should fix this for you. The issue was not so much the short array, as it was the self:: part of the referenced variable.

@umherirrender
Copy link
Author

Thanks for looking into this. It seems you found some more issues with the bitwise.

I had assumed the problem with the T_ARRAY, because the same code was working with the long array syntax. With the short array syntax it have only "problems" with the second and third bitwise. The first one was not reported. But fixing it proper by also checking the situation with self:: or an explicit class name would be nice.

@jrfnl
Copy link
Contributor

jrfnl commented Aug 13, 2017

@umherirrender You're welcome. This (and your other issue) looked like ones which the WP project would run into trouble with as well, so seemed like a good one to get fixed asap as WP is looking to fix the whole codebase in one go in the near future ;-)

I had assumed the problem with the T_ARRAY, because the same code was working with the long array syntax. With the short array syntax it have only "problems" with the second and third bitwise. The first one was not reported.

Well, short and long arrays were being treated differently in the original code, you were quite correct about that.

  • For long arrays, anything within them would be seen as a reference, which led to the bitwise and in array( 'a' => CONSTANT & OTHER_CONSTANT) being incorrectly identified as a reference.
  • For short arrays, if the & was directly following the opener (i.e. the first item) òr if it was following a comma and had a variable after it, it would correctly identify it as a reference, but that meant that &self::$something was incorrectly not recognized as a reference (the second and third item).

But fixing it proper by also checking the situation with self:: or an explicit class name would be nice.

That's what I'm hoping to achieve with the PR I opened, so fingers crossed that it'll be accepted.

@gsherwood gsherwood added this to the 3.1.1 milestone Sep 19, 2017
@gsherwood gsherwood changed the title File::isReference does not handle short array syntax the same as long array syntax File::isReference has problems with some bitwise operators and class property references Sep 25, 2017
gsherwood added a commit that referenced this issue Sep 25, 2017
wmfgerrit pushed a commit to wikimedia/mediawiki that referenced this issue Oct 23, 2017
Issue #1604 was fixed -
squizlabs/PHP_CodeSniffer#1604

Change-Id: Id81cb820e42123007ae8696422bebf588e274437
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants