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

Fix a number of apparent bugs and minor clean-up. #138

Merged
merged 3 commits into from Aug 3, 2016

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Aug 3, 2016

On a whim, I've run the codebase through Scrutinizer (without any specific config) and this threw up some interesting findings.

This PR handles most of these (for the sniff files).

Typical issues found & handled in this PR:

  • Returning false from the process() method. This is wrong. If the process method returns something it will be interpreted as an integer stack position to continue checking from, so returning false could lead to race conditions.
  • Variables which are set and subsequently never used.
  • Strict(er) checking of variables before/when using them
  • Function calls within loops
  • Minor documentation inconsistencies
  • A particular typo

Also:

  • Removed trailing spaces

One thing Scrutinizer threw up which is not (yet) handled is the following from the BaseSniffTest.php file:

    /**
     * Tear down after each test
     *
     * @return void
     */
    public function tearDown()
    {
        // Reset any settingsStandard (targetPhpVersion)
        self::$phpcs->cli->settingsStandard = array();
    }

The PHP_CodeSniffer_CLI class does not have a settingsStandard property, so this code is effectively useless. All the same, I imagine something like this does need to be done, so someone more familiar with the original intend of this code might want to have a look at what this should be replaced with.

@wimg
Copy link
Member

wimg commented Aug 3, 2016

Wow pretty amazing work. Sadly there seems to be a conflict preventing the merge :-(

Typical issues found & handled:
* Returning `false` from the `process()` method. This is wrong. If the process method returns something it will be interpreted as an integer stack position to continue checking from, so returning false could lead to race conditions.
* Variables which are set and subsequently never used.
* Strict(er) checking of variables before/when using them
* Function calls within loops
* Minor documentation inconsistencies
@jrfnl jrfnl force-pushed the feature/various-bug-fixes branch from 1ed8c9b to 3215a35 Compare August 3, 2016 22:29
@jrfnl
Copy link
Member Author

jrfnl commented Aug 3, 2016

Rebased, should be ok to merge now (once travis has finished).

@wimg wimg merged commit d30d887 into PHPCompatibility:master Aug 3, 2016
@jrfnl jrfnl deleted the feature/various-bug-fixes branch August 3, 2016 22:46
@jrfnl jrfnl added this to the 7.0.3 milestone Apr 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants