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

[4.x] Add PHPStan analysis to the build #197

Merged
merged 5 commits into from
Dec 22, 2017
Merged

Conversation

mhujer
Copy link
Contributor

@mhujer mhujer commented Dec 11, 2017

PHPStan is a PHP Static Analysis Tool.

I've fixed the errors it can find up to level 2 (it has 7 levels of checks), but this PR is already big enough, so I will create another one later on.

@@ -25,6 +25,7 @@ before_script:

script:
- ./vendor/bin/parallel-lint src tests
- composer run phpstan
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By calling the Composer script, it runs the same configuration as you would run locally.

@@ -37,6 +37,8 @@
"mockery/mockery": "^1.0",
"moontoast/math": "^1.1",
"php-mock/php-mock-phpunit": "^2.0",
"phpstan/phpstan-phpunit": "0.9.2",
"phpstan/phpstan-shim": "0.9.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the actual tool, but wrapped in a phar so it does not install any other dependencies

"test": [
"@lint",
"@phpstan",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be ran before tests, because it can discover issues that would make tests fail anyway

@@ -67,8 +69,13 @@
"lint": "parallel-lint src tests",
"phpunit": "phpunit --verbose --colors=always",
"phpcs": "phpcs src tests --standard=psr2 -sp --colors",
"phpstan": [
"@php vendor/phpstan/phpstan-shim/phpstan.phar analyse -c phpstan.neon src --level 2 --no-progress",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by splitting analysis of src and tests separate config file can be provided (e.g. with different ignored erros)

@@ -58,7 +58,7 @@ public function calculateTime($seconds, $microSeconds)
*/
public function convertTime($timestamp)
{
if (!$timestamp instanceof GMP) {
if (!$timestamp instanceof \GMP) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check if this is correct

Copy link
Owner

@ramsey ramsey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d like to understand more about what this does. Very little code has changed, but it looks like a lot of comments have been added. Does phpstan require these comments?

@mhujer
Copy link
Contributor Author

mhujer commented Dec 11, 2017

Most of the changes are for both PHPStorm and PHPStan (and anything that tries to analyze the code)

  1. /** @var Uuid $uuid */ comment is necessary anywhere, where the result of Uuid::fromString() is treated like the instance of Uuid. (::fromString() returns UuidInterface which lacks some of the methods available in Uuid)

eg. in the following code, the $uuid is UuidInterface, but the getClockSeqLow() method is available only in Uuid. The docblock makes it clear, that the Uuid is actually returned.

     public function testGetClockSeqLow()
      {
 +        /** @var Uuid $uuid */
          $uuid = Uuid::fromString('ff6f8cb0-c57d-11e1-9b21-0800200c9a66');
          $this->assertEquals(33, $uuid->getClockSeqLow());
      }
  1. In some places, it was necessary to add a phpdoc for a mock object, because methods from both objects can be called:
        /** @var \Ramsey\Uuid\Provider\Node\SystemNodeProvider|\PHPUnit_Framework_MockObject_MockObject $provider */
        $provider = $this->getMockBuilder(SystemNodeProvider::class)
            ->setMethods(['getIfconfig'])
            ->getMock();

        $provider->expects($this->once())  // method from \PHPUnit_Framework_MockObject_MockObject
            ->method('getIfconfig')
            ->willReturn(PHP_EOL . 'AA-BB-CC-DD-EE-FF' . PHP_EOL);

        $node = $provider->getNode(); // method from \Ramsey\Uuid\Provider\Node\SystemNodeProvider

$this->assertInstanceOf(
SodiumRandomGenerator::class,
$uuid->getFactory()->getRandomGenerator()
$actualUuidFactory->getRandomGenerator()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this change, but originally it was calling the static method non-statically.

@mhujer
Copy link
Contributor Author

mhujer commented Dec 11, 2017

PHPStan works by analyzing the types in the code and in the phpdocs and then it uses it to report potentially incorrect code. (It knows, that if the method accepts some type, you cannot pass other type there.) The analysis is possible by having proper types in the code (which also helps the PHPStorm and other IDEs).

If there is anything else I can explain, you can comment directly at the specific line of the diff.

@ramsey
Copy link
Owner

ramsey commented Dec 22, 2017

After considering this some more, I've decided to accept it. Thanks for all your hard work!

@ramsey ramsey merged commit 311dc8b into ramsey:4.x Dec 22, 2017
@ramsey
Copy link
Owner

ramsey commented Dec 22, 2017

✅ SemVer Sentry Result

This PR will not change anything and can be merged freely without breaking backwards compatibility.

If you decide to create a release after merging this PR, you should know that the changes in this PR warrants at least a PATCH version release.

SemVer Sentry is in beta, take it's recommendations with a grain of salt and if in doubt, double check. If you want to get in touch with us, use the contact form on our homepage.

See here for more information on semantic versioning (semver).

@mhujer mhujer deleted the mh-phpstan branch December 23, 2017 08:19
@ramsey ramsey mentioned this pull request Jan 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants