Skip to content
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

Fix GH-17951: Addition of max_memory_limit INI #18011

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

frederikpyt
Copy link

@frederikpyt frederikpyt commented Mar 10, 2025

Added a new INI max_memory_limit which acts as a cap on how much you can raise memory_limit using ini_set() during runtime.

Perhaps see: #17951

@iluuu1994
Copy link
Member

Looks good in general though. 👍

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.

LGTM otherwise, thank you!

main/main.c Outdated
@@ -317,11 +317,39 @@ static PHP_INI_MH(OnSetSerializePrecision)
static PHP_INI_MH(OnChangeMemoryLimit)
{
size_t value;

Copy link
Member

Choose a reason for hiding this comment

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

Nit: Please avoid unrelated whitespace changes.

main/main.c Outdated
@@ -332,7 +360,30 @@ static PHP_INI_MH(OnChangeMemoryLimit)
return FAILURE;
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Same

main/main.c Outdated
}
/* }}} */

/* {{{ PHP_INI_MH */
Copy link
Member

Choose a reason for hiding this comment

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

We decided to drop vim comments for new code

@iluuu1994
Copy link
Member

@frederikpyt Feel free to ping me again ~2 weeks after you've sent the e-mail to the internals mailing list. If there are no objections, this may be merged.

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