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

Add PHPStan #7883

Merged
merged 3 commits into from Feb 18, 2019

Conversation

Projects
None yet
5 participants
@CZechBoY
Copy link

CZechBoY commented Jan 8, 2019

Hi,
are you interested in adding PHPStan to your pipeline?

In this I added phpstan to your pipeline and fixed some issues (mostly in phpdoc). Some of reported issues are not fixed yet and I would like to fix it with your help.
In current state there is completely ignored error about ioncube functions not found and Symfony Console Terminal class not found (i see that usage of this class is always checked with class_exists).

@CZechBoY CZechBoY force-pushed the CZechBoY:phpstan branch from 5a2ece1 to c090147 Jan 8, 2019

.travis.yml Outdated
# Run PHPStan
- if [[ $PHPSTAN == "1" ]]; then
yes | pecl install imagick &&
(yes | pecl install rar || true) &&

This comment has been minimized.

@Nek-

Nek- Jan 8, 2019

Why? pecl may fail here??

This comment has been minimized.

@CZechBoY

CZechBoY Jan 8, 2019

Author

Yes, rar fails on php 7.3. Additionaly there is rar stub in phpstan/stubs/rar.php, so phpstan can analyze calls to rar library.

This comment has been minimized.

@staabm

staabm Feb 12, 2019

Contributor

can we just always rely on the stubs and dont install this extensions?

This comment has been minimized.

@ondrejmirtes

ondrejmirtes Feb 12, 2019

Please make sure that you're installing the extensions only for those extensions that are object-oriented and use classes. PHPStan does not need extensions in the environment that only provide functions, that's already covered in the core.

if (!extension_loaded('apcu')) {
require_once __DIR__ . '/stubs/apcu.php';
}

This comment has been minimized.

@Nek-

Nek- Jan 8, 2019

Is this test suite performance stuff? Maybe you should comment it.

This comment has been minimized.

@CZechBoY

CZechBoY Jan 8, 2019

Author

APCu is used in composer so phpstan need info about functions/classes in this extension. This command includes stubs for library from phpstorm-stubs cloned file.

This comment has been minimized.

@CZechBoY

CZechBoY Jan 8, 2019

Author

I don't know how to install php-apcu properly so I included this php stub.

@CZechBoY

This comment has been minimized.

Copy link
Author

CZechBoY commented Jan 8, 2019

See travis-ci pipeline for found errors/warnings from phpstan.

What is desired solution for this problem?

@CZechBoY

This comment has been minimized.

Copy link
Author

CZechBoY commented Feb 11, 2019

Any update here? Can I improve something?

@Nek- ?

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Feb 11, 2019

@CZechBoY I need to evaluate this further still.. We are doing massive refactorings in the 2.0 branch atm so I don't think it's very wise to merge this in master/1.x as it'll most likely cause conflicts.

If you don't mind rebasing this on the 2.0 branch that'd be a good start.

The way you did it starting with a level 0 run is good, we can then get it merged without too many changes to review and then look at increasing the level afterwards in follow-up PRs.

@CZechBoY

This comment has been minimized.

Copy link
Author

CZechBoY commented Feb 11, 2019

Thank for the reply.
No problem, I will rebase onto 2.0 branch and ping you then

@Seldaek Seldaek added this to the 2.0-core milestone Feb 12, 2019

@CZechBoY CZechBoY force-pushed the CZechBoY:phpstan branch from dd16c96 to 43d3d6d Feb 12, 2019

@CZechBoY CZechBoY changed the base branch from master to 2.0 Feb 12, 2019

@CZechBoY CZechBoY force-pushed the CZechBoY:phpstan branch from 43d3d6d to b1e58bd Feb 12, 2019

@CZechBoY

This comment has been minimized.

Copy link
Author

CZechBoY commented Feb 12, 2019

OK, I rebased onto 2.0 and changed target branch to 2.0.
There were some minor issues which were fixed. Some issues I cannot handle by myself so you can see them in CI job (or by local run) in fix them on your own.

ping @Seldaek @Nek-

@@ -0,0 +1,321 @@
<?php

This comment has been minimized.

@staabm

staabm Feb 12, 2019

Contributor

would it make sense to install these stubs via composer instead of copying them into the repos (that way those would automatically get upstream fixes/updates)

This comment has been minimized.

@CZechBoY

CZechBoY Feb 12, 2019

Author

You cannot install php extensions via composer.
I tried to install APCu into travis environment but failed. Feel free to add ;-)

This comment has been minimized.

@staabm

staabm Feb 12, 2019

Contributor

I am not saying we should try to install the extensions, but instead of installing them, always use the stubs - would this make sense?

This comment has been minimized.

@CZechBoY

CZechBoY Feb 12, 2019

Author

Proper installation of php extension into CI makes environment more production like. Stubs can be out of date, invalid, incomplete etc.

As pointed below, we can decide if we want to use stubs or ignore errors (and possible issues)
from rarely used extension completly.

This comment has been minimized.

@Seldaek

Seldaek Feb 12, 2019

Member

IMO ignoring is fine.. no need for stub or installing these. These exts are hardly used.

@staabm

This comment has been minimized.

Copy link
Contributor

staabm commented Feb 12, 2019

the travis build reports an error:

There was 1 error:
1) Composer\Test\DependencyResolver\SolverTest::testLearnPositiveLiteral
Trying to get property 'testFlagLearnedPositiveLiteral' of non-object
/home/travis/build/composer/composer/tests/Composer/Test/DependencyResolver/SolverTest.php:903

is it related to this PR?

@CZechBoY

This comment has been minimized.

Copy link
Author

CZechBoY commented Feb 12, 2019

@staabm because branch 2.0 fails here too (and this pr is rebased onto 2.0)
latest pipeline: https://travis-ci.org/composer/composer/builds/491331896
for actual pipeline of all branches see https://travis-ci.org/composer/composer/branches

@staabm

This comment has been minimized.

Copy link
Contributor

staabm commented Feb 12, 2019

one of the reported phpstan errors was resolved with #7978 and should be gone after rebase

@staabm

This comment has been minimized.

Copy link
Contributor

staabm commented Feb 12, 2019

I just opened 2 issues on upstream phpstan for the warnings, which I guess are a bug in phpstan itself.

@staabm

This comment has been minimized.

Copy link
Contributor

staabm commented Feb 12, 2019

so it turns out one of the upstream opened issue was not a bug on phpstan but one in composer 2.0 only.

------ ------------------------------------------------------- 
  Line   src/Composer/Downloader/PerforceDownloader.php         
 ------ ------------------------------------------------------- 
  81     Call to an undefined method                            
         Composer\Downloader\PerforceDownloader::doDownload().  
 ------ ------------------------------------------------------- 

I opened a separate issue for it:
#7979

@ondrejmirtes

This comment has been minimized.

Copy link

ondrejmirtes commented Feb 12, 2019

BTW I'd rather ignore the APCu and RAR errors than commit thousands of lines of stubs into the project.

@Seldaek Seldaek merged commit 5b0b88c into composer:2.0 Feb 18, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details

@CZechBoY CZechBoY deleted the CZechBoY:phpstan branch Feb 18, 2019

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Feb 18, 2019

Thanks - build failed though in a strange way https://travis-ci.org/composer/composer/jobs/494856647

If I add back those two lines in config.neon to fix the travis build:

        - '~^Instantiated class Symfony\\Component\\Console\\Terminal not found\.$~'
        - '~^Class Symfony\\Component\\Console\\Input\\StreamableInputInterface not found\.$~'

I get this error locally:

$ phpstan analyse src tests --configuration=phpstan/config.neon --autoload-file=phpstan/autoload.php
 356/356 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 --------------------------------------------
  Error
 --------------------------------------------
  Ignored error pattern ~^Instantiated class Symfony\\Component\\Console\\Terminal not found\.$~ was not matched in reported errors.
  Ignored error pattern ~^Class Symfony\\Component\\Console\\Input\\StreamableInputInterface not found\.$~ was not matched in reported errors.
 --------------------------------------------

 [ERROR] Found 2 errors

Any ideas?

@ondrejmirtes

This comment has been minimized.

Copy link

ondrejmirtes commented Feb 18, 2019

Looks like the build isn't entirely deterministic/repetable. You probably have different dependency versions installed locally?

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Feb 18, 2019

Hmm I guess the problem is I have phpstan installed in composer's global dir - which has a few of the latest symfony/* packages installed in there, so probably that means phpstan finds the missing classes in there.

I tried running it with phpstan installed as shim locally and that works, so will add back those exclusions to make the build pass I guess.

@ondrejmirtes

This comment has been minimized.

Copy link

ondrejmirtes commented Feb 18, 2019

Sounds that's it :) BTW let me know if you need any help with this - I'd love to see the PHPStan level increased, basically on level 0 it just looks for static errors and for existing methods called on $this - it's much more capable on higher levels :)

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Feb 18, 2019

Yes of course, the plan is to increase the level slowly as long as it yields sensible output. I don't really want to fight with a tool about nitpicks otherwise it becomes a negative return on investment, so we will see where the right level is.

@ondrejmirtes

This comment has been minimized.

Copy link

ondrejmirtes commented Feb 18, 2019

I'd say that level 5 is reasonable for every project - that's the level that starts checking types of arguments passed to functions and methods.

Level 6 and 7 are really strict and only for greenfield projects I'd say - they're much more strict about unions and nullables and the main problem is that they find issues in stuff that's usually only in developer's heads but not expressed in code - for example if phpDoc says Foo|Bar but you know it's always Foo, PHPStan on L6 will tell you "be careful, it might be Bar". Same thing with L7 - if your phpDoc or native typehint says that something might be |null but you know it isn't, PHPStan will still complain.

@CZechBoY

This comment has been minimized.

Copy link
Author

CZechBoY commented Feb 18, 2019

@Seldaek Try to install with lowest deps, and also install phpstan-shim, not full phpstan package with all dependencies.

@muglug muglug referenced this pull request Feb 18, 2019

Merged

Fix type issues #7996

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment