Super-linter: PHP Support

[lukeholder]

Possible linters:

• https://cs.symfony.com/ (9.1k Stars)
• https://psalm.dev/ (3.1k Stars)
• https://github.com/phpstan/phpstan (8.8k Stars)
• https://github.com/phan/phan (4.8k Stars)

[fafnirical]

https://github.com/squizlabs/PHP_CodeSniffer is another very popular tool (7.7K stars, 64.5M installs through Packagist)

[WyriHaximus]

Laid the foundation for this in #129 with just pure PHP syntax linting

[morrisonlevi]

By the way, linting is built into PHP with php -l, so all the project really needs to do is run that across all the PHP files. Looks like the PR tries to do this in parallel. That's probably fine if other linters run in parallel too, but otherwise I'd simplify it to remove dependencies.

[WyriHaximus]

By the way, linting is built into PHP with php -l, so all the project really needs to do is run that across all the PHP files. Looks like the PR tries to do this in parallel. That's probably fine if other linters run in parallel too, but otherwise I'd simplify it to remove dependencies.

Doesn't matter anyway. Just had a good look at how the linter works and it checks file by file so it can't even utilize the parallelization :(.

[fafnirical]

129 looks like a great foundation for PHP that I think we could build from. php -l performs only syntax checking, while many of the other linters in this project run both syntax and stylistic checks

[ConradSollitt]

Personally for PHP I'm in favor of using the submitted php -l option over the other popular linters by default. Or if supporting multiple linters at least include the php -l option. I do see this project allows for multiple linters per language.

The reason is because PHP linters feel very opinionated to me compared to linters in other languages (particaullary JavaScript) and result in a lot of false positives. I ended up using phpstan on a project with about ~19,000 lines of PHP code https://github.com/fastsitephp/fastsitephp/blob/master/phpstan.neon and had to add a lot of exceptions to get the code to lint. All of the exceptions were valid as the project has more lines 25,000 lines of code for Unit Testing and specifically tested many of the lines I had to make exceptions for.

Although as I write this it would be nice for my project to lint with both phpstan and php -l and for many projects they would want one of the other popular linters.

[ondrejmirtes]

PHPStan on higher rule levels finds valid issues from the type system point of view. If it says Cannot call method foo() on Bar|null then you might have a potential bug when the method callee is a null value and not the expected object.

But on lower rule levels it aims to find only 100 % crashing bugs in the analyzed codebase. So the recommended approach is to start on level 0, fix all bugs and then raise the level, rinse and repeat. If it finds too many errors you don't agree with, you've chosen a rule level that's too high for the current state of the analyzed codebase.

Also there's the baseline feature which will basically generate an ignorelist for the current state of the codebase, so if you want to integrate PHPStan into your code and bring changed and newly written code under more control, you can choose a higher level, generate the baseline and enjoy stricter checks for pull requests that come from that point in time onward.

But as the creator of PHPStan, I agree that php -l is the obvious choice for this new SuperLinter tool. Adding PHPStan to your CI pipeline and the project is already pretty easy so anyone can do it even if it's not part of this umbrella tool.

[WyriHaximus]

And this is why I only started syntaxt, because the other "linters" listed are more then just linters. They are static analysis tools, code style checks, and other quality assurance tooling. Which is awesome to add, but comes with a ton of configuration each. I'll leave it up to this project if that is desired or not, if it is, we can easily add all of them and only trigger when their configuration file is present.

[ConradSollitt]

@ondrejmirtes Thanks for the feedback and great job on PHPStan. I ended up using it on a somewhat large PHP project that I wrote over 6 years https://github.com/fastsitephp/fastsitephp and later I plan on documenting the process and how it works more in the future but basically the project was already created with extensive unit testing before I added the PHP linting so I carefully compared all popular PHP linters for several days.

I ended up liking PHPStan the best out of all of the ones I tested (psalm, phan) so I selected it for my project and started at level 0 then went all the way to level 8 over a period of a few days.

[fafnirical]

This project's stated goals are:

1. Prevent broken code from being uploaded to the default branch
2. Help establish coding best practices across multiple languages
3. Build guidelines for code layout and format
4. Automate the process to help streamline code reviews

php -l handles the first goal well as a syntax checker. I believe another tool is necessary to handle the second and third goals. Unfortunately, there doesn't seem to be much consensus around what tool that should be: Symfony uses PHP Coding Standards Fixer and PHPStan; Laravel uses StyleCI; Drupal, CodeIgniter, and CakePHP use PHP_CodeSniffer (CakePHP uses PHPStan as well)

[ConradSollitt]

Great input @fafnirical and examples on how popular php frameworks use different linters.

I think reasons like this (wide use of different linters and PHP Static Analysis Tools) are valid reasons that all could be supported as an option here. Currently JavaScript supports two linters with this project so supporting multiple linters is a valid option at least.

[divinity76]

'php -l' is not a linter, it's only a syntax checker, it wouldn't even catch '''

Yeah that's not a linter. (Sorry about shitty formatting, am on phone)

[ConradSollitt]

Great example @divinity76

PHP is definitely a challenge on how to handle in my option due to large number of ways to run it and the fact that it uses both errors that are triggered and exceptions that are thrown.

Personally I prefer always handling errors and re-throwing them as exceptions however as the example you provide php -l will not catch this because calling a function is a runt-time error and not a compiler error. Some apps will actually allow error warnings on the page, I recall often seeing many warnings on production WordPress sites years ago.

I expanded on your example a little bit and just tested with PHP 5.6 and 7.1. Both 5.6 and 7.1 consider the file valid and will attempt to run it and show no errors when running with HANDLE_ERRORS = false. When HANDLE_ERRORS = true then PHP 5.6 will trigger an error while PHP 7.1 will throw an Exception of TypeError.

~~~php
const HANDLE_ERRORS = true;
if (HANDLE_ERRORS) {
error_reporting(-1);

set_exception_handler(function($e) {
    var_dump($e);
});

set_error_handler(function($severity, $message, $file, $line) {
    throw new \ErrorException($message, 0, $severity, $file, $line);
});

}

echo phpversion();
echo PHP_EOL;

function f(int $i){
echo $i;
echo PHP_EOL;
}
f(123);
f("not int");
f(123);
~~~

The fact that php -l works on this and other error code could be a valid reason to support all popular PHP syntax checkers. At least php -l can help catch syntax errors that prevent a file from compiling.

Another thing I was thinking of is that just php -l should be supported as it handles compile errors and then it's up to the actual app to handle Unit Testing.

[ondrejmirtes]

I don’t think that anyone is making type errors on purpose, catching them and doing custom logic in their error handler 😊

The point of @divinity76 was I believe that besides php -l there are other static analysers that would find the aforementioned error very easily. Also, in PHP you don’t have to write unit tests for things that can be caught by the type system, it’s not a productive way to spend your time.

I wrote about this a few years ago: https://www.phparch.com/2018/04/testing-strategy-with-the-help-of-static-analysis/

[WyriHaximus]

@ondrejmirtes correct me if I'm wrong, but PHPStan would catch mistakes like that right, without complaining about taking anti-patterns it does at higher levels? Because I agree with @divinity76 as well, php -l was merely a first step. Haven't looked into adding PHPStan, Psalm, PHP CS Fixer and such yet.

[ConradSollitt]

@ondrejmirtes @WyriHaximus @divinity76

Yeah I agree it would be nice to have PHPStan and/or the other popular PHP Syntax Checkers as well. I think php -l will really only help someone if they check in code with out doing any sort of validation and make major syntax errors.

[ondrejmirtes]

PHPStan tries not to be opinionated and looks for legit bugs only. Of course some errors it points out are errors only in PHPDocs but that’s useful anyway. Level 5 strikes a good balance and it’d definitely catch the issue mentioned above.

I don’t know how to run it though - to work well, it needs to be executed on the whole codebase at once, it’s not much useful to run it on single files.

I also already stated that php -l is fine for this purpose. It’s already easy to run PHPStan separately, thousands of repositories already do it, so I don’t know how useful is it to spend time figuring out how to bend it for an umbrella tool like SuperLinter is.

[ondrejmirtes]

Overview of PHPStan’s rule levels is here: https://phpstan.org/user-guide/rule-levels

[admiralAwkbar]

PHP support was added in #244

[divinity76]

@admiralAwkbar that's not a linter, that's purely a syntax checker, see my first comment above =/

[filips123]

Are there any plans to add other linters mentioned in this issue in the future? JavaScript, TypeScript and Terraform already have multiple linters supported so I don't see why PHP also shouldn't have.

[divinity76]

sigh, as far as PHP goes, it should be named super-shitty-linter

this "super linter" won't catch this error, plz fix:

<?php
function f(int $i){}
f("not int");

| ping @admiralAwkbar

[WyriHaximus]

@divinity76 It's an open source project, you can always add it and open a PR :wink:

@filips123 let me have a look, if support for more than one is in it shouldn't be to hard to add PHPStan, Psalm, etc

[divinity76]

@WyriHaximus 5 different linters have been proposed above, *all* of which would catch the error,

what has been implemented is NOT a linter, it's a syntax checker.

as far as linting goes, "super-linter" still doesn't have PHP support for linting.

i don't think this issue is ready to be closed.

[admiralAwkbar]

Re-opened the issue :)

@divinity76 @WyriHaximus @@filips123
If someone wants to propose a PR with one of the above-selected tools, we would be glad to implement...
As we can see from others like python, they have pylint and flake8 support. Nothing is stopping us from having more than 1 linter for the language and users can enable what they feel meets their standards. Please open a PR and I would be glad to help move this along.

thanks!

[WyriHaximus]

@admiralAwkbar :+1: , Currently working on a PR for PHPStan

[filips123]

I will probably add PHP_CodeSniffer and Psalm but I will wait for @WyriHaximus's PR to see how to handle external PHP dependencies.

[WyriHaximus]

@filips123 IIRC they all have phars so they shouldn't be to hard. Otherwise I'll add Composer and install it that way

[WyriHaximus]

@filips123 all have phars :tada:

[WyriHaximus]

@filips123 @divinity76 draft PR for PHPStan is up, need to add a few more things but will do that tomorrow: https://github.com/github/super-linter/pull/477

[filips123]

I will probably do PRs today. And for checking signatures, we could use Phive which automatically downloads and verficies PHARs. I think that it supports most linters mentioned here.

[WyriHaximus]

@filips123 Ow installing with phive it pretty well done, just no clue how to run phpstan now tbh

[filips123]

By default it save phars into some common directory (I think ~/.phive/phars) and symlink them into ./tools, so PHPStan will be in ./tools/phpstan. If you install them globally phive install -g, they will be symlinked into /usr/local/bin which is most likely already in path so you can just use phpstan to run it. Symlink will probably work, but in case it doesn't, you can use --copy to just copy phars.

I already partly added PHPCS and Phive using Phive, but will probably create PR tomorrow because it is not ready yet.

[WyriHaximus]

Nice thanks, came to this solution: https://github.com/github/super-linter/pull/477/files#diff-3254677a7917c6c01f55212f86c57fbfR203

But the -g might be even better

[filips123]

You can probably have dependencies/phive.xml with all supported linters, then just copy it to /root/.phive/global.xml and run phive install -g to install all Phars.

phive.xml needs to look something like this:

<?xml version="1.0" encoding="UTF-8"?>
<phive xmlns="https://phar.io/phive">
    <phar name="phpcs" version="^3.5" installed="3.5.5" location="/usr/local/bin/phpcs" copy="true"/>
    <phar name="psalm" version="^3.12" installed="3.12.2" location="/usr/local/bin/psalm" copy="true"/>
</phive>

[WyriHaximus]

-g works great, got a phpstan file in there as well, the only thing that bugs me is that it doesn't pick it up from the root of a repo