Skip to content

Prevent FPM tests from failing due to expose_php #9508

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

Closed
wants to merge 1 commit into from
Closed

Prevent FPM tests from failing due to expose_php #9508

wants to merge 1 commit into from

Conversation

dwxh
Copy link
Contributor

@dwxh dwxh commented Sep 8, 2022

If a php.ini is installed in $PREFIX containing expose_php = off, a number of FPM tests fail due to a missing-but-expected X-Powered-By header. This change injects an "expose_php = on" into the fpm pool config.

This fails to fix one test - sapi/fpm/tests/status-listen.phpt - which continues to fail as the status pool doesn't pick up php_flag options from the fpm pool config. I'll see about applying php_ flags/values to the status pool, but leave that for a separate pull request as it's a more significant change.

If a php.ini is installed in $PREFIX containing expose_php = off,
a number of FPM tests fail due to a missing-but-expected
X-Powered-By header. This change injects an "expose_php = on"
into the fpm pool config.
@cmb69
Copy link
Member

cmb69 commented Sep 8, 2022

Thanks for the PR! I don't know much about FPM, but would it be possible and sensible to use a --INI-- section to set this directive?

@dwxh
Copy link
Contributor Author

dwxh commented Sep 8, 2022

I don't think that would work - this is getting injected into the config for a new FPM pool rather than the tester's php process, and I don't think it gets inherited.

I did try:

--TEST--
FPM: Status basic test
--INI--
expose_php=On

And that didn't do it, but it's possible I got the syntax wrong?

@cmb69
Copy link
Member

cmb69 commented Sep 8, 2022

And that didn't do it, but it's possible I got the syntax wrong?

No, the syntax should be okay. So we likely want to stick with updating the FPM configuration.

@bukka
Copy link
Member

bukka commented Sep 29, 2022

Thanks for reporting this and trying to fix it. I went for slightly less invasive version at the end but good that you tried this because it showed the problem with status_listen shared pool. Will comment on it in that other PR.

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

Successfully merging this pull request may close these issues.

3 participants