Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Fix ini values not getting restored after request has finished #19964

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

Open
manuelm wants to merge 3 commits into php:PHP-8.5
base: PHP-8.5
Choose a base branch
Loading
from manuelm:gh17951

Conversation

Copy link
Contributor

@manuelm manuelm commented Sep 25, 2025 •
edited
Loading

This fixes two separate cases of ini values not getting restored:

  • The max_memory_limit handler directly overwrites the global memory_limit value.
  • The fpm sapi does not restore any ini values passed through fcgi environment (PHP_VALUE header).

Both cases should be passed through zend_alter_ini, so modified_ini_directives can keep track.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OnChangeMaxMemoryLimit part makes sense to me. I did not know per-host ini settings were a thing, and could change OnChangeMaxMemoryLimit after OnChangeMemoryLimit.

As for the FPM part, I'll let that review up to Jakub, as I don't know much about FPM.

Copy link
Member

iluuu1994 commented Oct 6, 2025 •
edited
Loading

@bukka Can you please verify the sapi/fpm/fpm/fpm_main.c part? Alternatively, we may split the PR so I can merge the fix in main/main.c in the meantime.

@manuelm Weren't there tests before? Were they dropped purposefully?

Copy link
Member

bukka commented Oct 6, 2025

Yeah I got it on my list and should be able to check in the next few days.
iluuu1994 reacted with thumbs up emoji

Copy link
Contributor Author

manuelm commented Oct 6, 2025 •
edited
Loading

@manuelm Weren't there tests before? Were they dropped purposefully?

My bad. I somehow missed them in the rebase. They are back now.
iluuu1994 reacted with thumbs up emoji

Copy link
Member

Argh... Referenced wrong commit in a different PR. Sorry about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iluuu1994 iluuu1994 iluuu1994 left review comments

@bukka bukka Awaiting requested review from bukka bukka is a code owner

@dstogov dstogov Awaiting requested review from dstogov dstogov is a code owner

@kocsismate kocsismate Awaiting requested review from kocsismate kocsismate is a code owner

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /