-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Stop shutdown handler outputting in child processes #6411
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
base: 12.4
Are you sure you want to change the base?
Stop shutdown handler outputting in child processes #6411
Conversation
|
@staabm Can you have a look at this? Thanks! |
composer.json
Outdated
| }, | ||
| "require-dev": { | ||
| "ext-pcntl": "*" | ||
| }, |
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.
composer.lock needs to be updated after composer.json is changed. This is the reason why the CI pipeline failed.
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've removed it as suggested in #6411 (comment)
| @@ -0,0 +1,23 @@ | |||
| --TEST-- | |||
| ShutdownHandler does not output when child process exits | |||
| --FILE-- | |||
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.
This needs a --SKIPIF-- section to skip the test when the pcntl extension is not available.
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.
Also note that we might not set up PHP with the pcntl extension in our CI pipeline yet.
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.
Also note that we might not set up PHP with the
pcntlextension in our CI pipeline yet.
we need this change in earlier release branches. therefore I am doing it here: #6412
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've added the --SKIPIF--
| @@ -0,0 +1,23 @@ | |||
| --TEST-- | |||
| ShutdownHandler does not output when child process exits | |||
| --FILE-- | |||
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.
needs a SKIPIF whether pcntl extension is available
composer.json
Outdated
| "require-dev": { | ||
| "ext-pcntl": "*" | ||
| }, |
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 think we need this change, because we need pcntl only for this single test but not for general development of phpunit.
getmypid() seems available even without pcntl.
we already have other tests depending on pcntl without this setting
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.
OK, done. Yes, getmypid() is part of the core.
| $_SERVER['argv'][] = '--filter'; | ||
| $_SERVER['argv'][] = 'testChildProcessOutput'; |
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.
we can drop the --filter testChildProcessOutput - it's unnecessary
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
staabm
left a comment
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.
lgtm - thanks
This fixes #6410