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
Test phpstan /w restricted open_basedir and disabled proc_open #7518
Conversation
@@ -78,6 +78,8 @@ jobs: | |||
git checkout ceadf6ea68c5ef316abfc618879f2cf9290e45b3 | |||
composer install | |||
../../../phpstan.phar analyse | |||
# test phpstan/4147 (restricted open_basedir) and phpstan-src/1451 (disabled proc_open) | |||
php -d open_basedir="$(pwd):/tmp" -d disable_functions="pcntl_exec,pcntl_fork,exec,passthru,proc_open,shell_exec,system,popen" ../../../phpstan.phar analyse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate matrix item please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if it will checkout (and composer install) the Lavarel twice then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to test it with Laravel, just create a new directory with a small "project" specific to this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No because of two things:
- larger project is expected to use at least one preload using composer
autoload/files
(https://github.com/laravel/framework/blob/ceadf6ea68c5ef316abfc618879f2cf9290e45b3/composer.json#L106) - and because only a larger project will generate multiple parallel jobs.
for ex. the https://github.com/slevomat/coding-standard.git is already analysed twice in .github/workflows/integration-tests.yml
/w different configurations
with this reasoning, is it ok to keep the 2nd reanalyse of Lavarel here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to misuse Laravel for this, I'm testing Laravel for a different reason. The assumption might change for a different reason.
just create a new directory with a small "project" specific to this test.
You can force multiple child processes for a smaller amount of files if you tweak these settings https://phpstan.org/config-reference#parallel-processing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, all feedback addressed. phpstan/phpstan-src#1466 must be merged first.
68dad27
to
84e23ec
Compare
b2a431e
to
52da149
Compare
52da149
to
ecc5a05
Compare
Thank you. |
related with phpstan/phpstan-src#1451