search

oxsecurity / megalinter

🦙 MegaLinter analyzes 50 languages, 22 formats, 21 tooling formats, excessive copy-pastes, spelling mistakes and security issues in your repository sources with a GitHub Action, other CI tools or locally.
https://megalinter.io
GNU Affero General Public License v3.0
1.81k stars 215 forks source link

[PHP_PHPSTAN_FILTER_REGEX_(IN|EX)CLUDE] variable override phpstan config file options #725

Closed llaville closed 2 years ago

llaville commented 2 years ago

Describe the bug PHPStan phpstan.neon config file parameters.paths and parameters.excludePaths options are overrided by [PHP_PHPSTAN_FILTERREGEX(IN|EX)CLUDE] variables

To Reproduce Steps to reproduce the behavior:

Following https://github.com/nvuillam/mega-linter/blob/master/docs/descriptors/php_phpstan.md#readme

Expected behavior Only src directory must be scanned to lint for *.php files (accordingly to PHP_PHPSTAN_FILE_EXTENSIONS)

Screenshots

[Filters] {'name': 'PHP_PHPSTAN', 'filter_regex_include': None, 'filter_regex_exclude': None, 'files_sub_directory': None, 'lint_all_files': False, 'lint_all_other_linters_files': False, 'file_extensions': ['.php'], 'file_names_regex': [], 'file_names_not_ends_with': [], 'file_contains_regex': []}
PHP_PHPSTAN linter files after applying linter filters:
- /tmp/lint/examples/basic.php
- /tmp/lint/examples/filters.php
- /tmp/lint/src/CallbackFilterHandler.php
- /tmp/lint/tests/CallbackFilterHandlerTest.php
- /tmp/lint/tests/TestCase.php
- /tmp/lint/tests/TestHandler.php
- /tmp/lint/tests/bootstrap.php

Additional context Run with latest version v4.45.0 and following command: docker run -v $(pwd):/tmp/lint nvuillam/mega-linter-php:v4

Workaround To fix this issue, we must specify variable as follow PHP_PHPSTAN_FILTER_REGEX_INCLUDE: "(src)"

llaville commented 2 years ago

If needed, here is a real use case (with workaround applied) available https://github.com/llaville/monolog-callbackfilterhandler/actions/runs/1209267988

nvuillam commented 2 years ago

Linter call command can be built using 3 modes (defined in Mega-Linter linters descriptors) :

As PHPSTAN is defined with file, the linter call is like : phpstan file1.php , phpstan file2.php, phpstan file3.php

I think that in this context, as we provide a file name, phpstan ignores its own config path and exclude_paths

I propose you an evolution allowing to override descriptor cli_lint_mode

Something like PHP_PHPSTAN_CLI_LINT_MODE: project , so FILTER_REGEX(IN/EXCLUDE would not be taken in account, and phpstan own config would be

Would that be ok for you ?

Note: do you know if PHPSTAN can take a list of files as parameter ? It would dramatically improve performances if i can set it to list_of_files by default ^^

llaville commented 2 years ago

The following is TRUE with at least PHPStan - PHP Static Analysis Tool 0.12.98

phpstan is able to include and/or exclude files from an external config file (with priority 1 - phpstan.neon, 2 - phpstan.neon.dist) This can be override by CLI arguments, even if config file is specified. For example.

With config file phpstan.neon

parameters:
    level: 6
    paths:
        - src/
        - examples/

  1. When we run phpstan analyse -c phpstan.neon, it will analyse all PHP files into src/ and examples directories

  2. When we run phpstan analyse -c phpstan.neon examples/, it will analyse only PHP files into examples directories (excluding src/)

  3. When we run phpstan analyse -c phpstan.neon tests/, it will analyse all PHP files into tests/ directory (excluding src/ and examples/)

  4. Is able to analyse a list of files with option --paths-file; for example: phpstan analyse --paths-file=/tmp/php_phpstan

Beware than paths are relative to location of "paths-file". And of course config file is override as previous when using --paths-file option

Hope that will help to understand how phpstan works

llaville commented 2 years ago

Linter call command can be built using 3 modes (defined in Mega-Linter linters descriptors) :

Thanks to point me this feature, I don't know yet. Really appreciated that you take time to reply and provide such level of information !

TLDR;

I propose you an evolution allowing to override descriptor cli_lint_mode

Something like PHP_PHPSTAN_CLI_LINT_MODE: project , so FILTER_REGEX(IN/EXCLUDE would not be taken in account, and phpstan own config would be

Would that be ok for you ?

I've just tested the "project" mode with PHPStan. Here are my results with adding an error to see what happens :

Before to add the cli_lint_mode: project in https://github.com/nvuillam/mega-linter/blob/v4.45.0/megalinter/descriptors/php.megalinter-descriptor.yml#L77-L91

With current v4.45, and my workaround solution

### Processed [PHP] files
- Using [phpstan v0.12.98] https://phpstan.org/
- Mega-Linter key: [PHP_PHPSTAN]
- Rules config: [/.github/linters/phpstan.neon.dist]
[phpstan] examples/basic.php - SUCCESS - 0 error(s)
[phpstan] examples/filters.php - SUCCESS - 0 error(s)
[phpstan] src/CallbackFilterHandler.php - ERROR - 1 error(s)
--Error detail:
 ------ ----------------------------------------------------------------------- 
  Line   CallbackFilterHandler.php                                              
 ------ ----------------------------------------------------------------------- 
  81     Parameter #1 $level of method                                          
         Monolog\Handler\AbstractHandler::__construct() expects                 
         100|200|250|300|400|500|550|600|'ALERT'|'alert'|'CRITICAL'|'critical'  
         |'DEBUG'|'debug'|'EMERGENCY'|'emergency'|'error'|'ERROR'|'INFO'|'info  
         '|'NOTICE'|'notice'|'WARNING'|'warning', int|string given.             
 ------ ----------------------------------------------------------------------- 

 [ERROR] Found 1 error                                                          

❌ Linted [PHP] files with [phpstan]: Found 1 error(s) - (2.67s)

+----SUMMARY------+----------------------+-------+-------+--------+--------------+
| Descriptor      | Linter               | Files | Fixed | Errors | Elapsed time |
+-----------------+----------------------+-------+-------+--------+--------------+
| ✅ EDITORCONFIG | editorconfig-checker |    15 |       |      0 |        3.96s |
| ✅ MARKDOWN     | markdownlint         |     2 |       |      0 |        0.39s |
| ✅ MARKDOWN     | markdown-link-check  |     2 |       |      0 |        5.29s |
| ✅ PHP          | php                  |     7 |       |      0 |        0.38s |
| ✅ PHP          | phpcs                |     7 |       |      0 |        0.81s |
| ❌ PHP          | phpstan              |     3 |       |      1 |        2.67s |
| ✅ YAML         | prettier             |     1 |       |      0 |        0.64s |
| ✅ YAML         | yamllint             |     1 |       |      0 |        0.22s |
+-----------------+----------------------+-------+-------+--------+--------------+

And now with PHPStan in project's mode


### Processed [PHP] files
- Using [phpstan v0.12.96] https://phpstan.org/
- Mega-Linter key: [PHP_PHPSTAN]
- Rules config: [/.github/linters/phpstan.neon.dist]
--Error detail:
 ------ ----------------------------------------------------------------------- 
  Line   src/CallbackFilterHandler.php                                          
 ------ ----------------------------------------------------------------------- 
  81     Parameter #1 $level of method                                          
         Monolog\Handler\AbstractHandler::__construct() expects                 
         100|200|250|300|400|500|550|600|'ALERT'|'alert'|'CRITICAL'|'critical'  
         |'DEBUG'|'debug'|'EMERGENCY'|'emergency'|'error'|'ERROR'|'INFO'|'info  
         '|'NOTICE'|'notice'|'WARNING'|'warning', int|string given.             
 ------ ----------------------------------------------------------------------- 

 [ERROR] Found 1 error                                                          

❌ Linted [PHP] files with [phpstan]: Found 1 error(s) - (1.42s)

+----SUMMARY------+----------------------+---------+-------+--------+--------------+
| Descriptor      | Linter               |   Files | Fixed | Errors | Elapsed time |
+-----------------+----------------------+---------+-------+--------+--------------+
| ✅ EDITORCONFIG | editorconfig-checker |      15 |       |      0 |        3.76s |
| ✅ MARKDOWN     | markdownlint         |       2 |       |      0 |        0.38s |
| ✅ MARKDOWN     | markdown-link-check  |       2 |       |      0 |        5.62s |
| ✅ PHP          | php                  |       7 |       |      0 |        0.37s |
| ✅ PHP          | phpcs                |       7 |       |      0 |        0.86s |
| ❌ PHP          | phpstan              | project |       |      1 |         1.4s |
| ✅ YAML         | prettier             |       1 |       |      0 |        0.66s |
| ✅ YAML         | yamllint             |       1 |       |      0 |        0.23s |
+-----------------+----------------------+---------+-------+--------+--------------+

OK we see errors but we don't know how much files were proceeded !

We can have this information by specify the --debug argument in PHPStan command line: PHP_PHPSTAN_ARGUMENTS: "--debug"

That give such results 


### Processed [PHP] files
- Using [phpstan v0.12.96] https://phpstan.org/
- Mega-Linter key: [PHP_PHPSTAN]
- Rules config: [/.github/linters/phpstan.neon.dist]
--Error detail:
/tmp/lint/src/CallbackFilterHandler.php
/tmp/lint/examples/filters.php
/tmp/lint/examples/basic.php
 ------ ----------------------------------------------------------------------- 
  Line   src/CallbackFilterHandler.php                                          
 ------ ----------------------------------------------------------------------- 
  81     Parameter #1 $level of method                                          
         Monolog\Handler\AbstractHandler::__construct() expects                 
         100|200|250|300|400|500|550|600|'ALERT'|'alert'|'CRITICAL'|'critical'  
         |'DEBUG'|'debug'|'EMERGENCY'|'emergency'|'error'|'ERROR'|'INFO'|'info  
         '|'NOTICE'|'notice'|'WARNING'|'warning', int|string given.             
 ------ ----------------------------------------------------------------------- 

 [ERROR] Found 1 error                                                          

❌ Linted [PHP] files with [phpstan]: Found 1 error(s) - (1.43s)

+----SUMMARY------+----------------------+---------+-------+--------+--------------+
| Descriptor      | Linter               |   Files | Fixed | Errors | Elapsed time |
+-----------------+----------------------+---------+-------+--------+--------------+
| ✅ EDITORCONFIG | editorconfig-checker |      15 |       |      0 |        3.76s |
| ✅ MARKDOWN     | markdownlint         |       2 |       |      0 |        0.38s |
| ✅ MARKDOWN     | markdown-link-check  |       2 |       |      0 |        5.62s |
| ✅ PHP          | php                  |       7 |       |      0 |        0.37s |
| ✅ PHP          | phpcs                |       7 |       |      0 |        0.86s |
| ❌ PHP          | phpstan              | project |       |      1 |         1.4s |
| ✅ YAML         | prettier             |       1 |       |      0 |        0.66s |
| ✅ YAML         | yamllint             |       1 |       |      0 |        0.23s |
+-----------------+----------------------+---------+-------+--------+--------------+

I've tested with PHPStan argument --debug always active, with and without "project" mode, there are no impact on final result (at least tested with console reporter). Did you think we can activate the "project" mode and add the debug mode by default in PHPStan descriptor ? Are you agree with it ?

llaville commented 2 years ago

CAUTION about "project" mode.

When it's switch OFF and PHPStan config file does not exists (in project) and not specified by PHP_PHPSTAN_CONFIG_FILE variable, it's scanned all PHP files (following PHP_PHPSTAN_FILTER_REGEX_INCLUDE rule)

When it's switch ON and PHPStan config file does not exists (in project) and not specified by PHP_PHPSTAN_CONFIG_FILE variable, we get an error returned by PHPStan itself 

### Processed [PHP] files
- Using [phpstan v0.12.96] https://phpstan.org/
- Mega-Linter key: [PHP_PHPSTAN]
- Rules config: [phpstan.neon]
--Error detail:
At least one path must be specified to analyse.

You should think about a fallback strategy in such condition !

NOTICE And I've also noticed something strange (with LOG_LEVEL: DEBUG) when PHP_PHPSTAN_CONFIG_FILE is not set.

[phpstan] command: ['phpstan', 'analyse', '--no-progress', '--no-ansi', '--memory-limit', '1G', '-c', '/action/lib/.automation/phpstan.neon', '/tmp/lint/examples/basic.php']

Is the path to config file correct ?

nvuillam commented 2 years ago

Do we still have an issue here ? :)

llaville commented 2 years ago

Do we still have an issue here ? :)

Yes: I'll prepare a summary report of situation

llaville commented 2 years ago

Good news, while I'm writing the summary report, I found a solution that could solved many of problem, including #726 ! I'm able to make PR soon too (probably it will be ready for tomorrow morning) fixing #726 and this issue.

@nvuillam I want your agreement to rename default config file TEMPLATES/phpstan.neon to TEMPLATES/phpstan.neon.dist accordingly to PHPStan resolution priority

llaville commented 2 years ago

Long discussion was just opened https://github.com/llaville/docker-php-toolbox/discussions/9

It detailed problems found, and how to solve it ... PR will come tomorrow morning now !

nvuillam commented 2 years ago

@llaville ok for the renaming of the default PHPSTAN config file :)

llaville commented 2 years ago

@nvuillam Do you want separates PR to fix PHPStan rename config file, fix #726, and solution to fix this one ? Or a single one is enough ? Currently only new config filename is available on https://github.com/llaville/mega-linter/tree/phpstan_neon

llaville commented 2 years ago

PR #809 contains only PHPStan config file naming change.

llaville commented 2 years ago

@nvuillam FYI: I'll propose my last PR after 817 resolution for a better comparaison on how to fix this issue for all modes.

This last PR will include both a fix for this issue and #726 !

If you want to have a preview, it's explained at https://github.com/llaville/docker-php-toolbox/discussions/9#discussioncomment-1441725

llaville commented 2 years ago

Due to PRE_COMMANDS behavior with linter config file detection explained at https://github.com/nvuillam/mega-linter/issues/819#issuecomment-938573847, I cannot propose my last PR to fix this issue :(

github-actions[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.