Skip to content

Add memory_get_limit() function #8380

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

Conversation

iluuu1994
Copy link
Member

Closes GH-7944

@bwoebi
Copy link
Member

bwoebi commented Apr 15, 2022

I totally don't oppose that function, but I wonder whether we can just have a function giving the properly converted ini value.

E.g. (name is up to discussion) ini_value("memory_limit") would return the appropriate integer. This make it trivial for all INIs at once to get the true value, also boolean values having like On, Off, 1, 0, true, false etc would just return the proper bool.

@nikic
Copy link
Member

nikic commented Apr 15, 2022

Agree with @bwoebi conceptually, but I'm not sure this is really technically feasible. For STD_PHP_INI_ENTRY we do know where the value is stored and could fetch it. memory_limit in particular is a PHP_INI_ENTRY going though OnChangeMemoryLimit though, so we don't have any programmatic way to fetch the actual value.

@bwoebi
Copy link
Member

bwoebi commented Apr 15, 2022

We could add a mechanism for getting the value as a zval though (some function pointer). For most usages internally it would be totally non-breaking, because nearly everything uses a simple STD_PHP_INI_ENTRY.

@derickr
Copy link
Member

derickr commented Apr 22, 2022

I'm wondering whether to add a whole mechanism to deal with every INI setting isn't over-engineering for this specific case? Has anybody asked for this for other INI settings? ini_get() is usually good enough.

@bwoebi
Copy link
Member

bwoebi commented Apr 22, 2022

@derickr I can only talk for myself, but I've pushed at least once recently a bug into production because I did if (ini_get("someini")) ... which incidentally recognizes "0" as false and it passes on my local environment (but doesn't recognize Off etc. properly).

It's less "this is needed". More "this would be nice to have". Additionally I have one more specific use case: in ext/ddtrace we have quite a bunch of ini settings, of which some inis are parsed as arrays in specific ways. Such a function would enable exposing the internal array trivially instead of via an extra helper function.

So no, it's not just this specific case.

@cmb69
Copy link
Member

cmb69 commented Apr 22, 2022

Has anybody asked for this for other INI settings?

@rquadling already asked for this long time ago; and there is a somewhat related request regarding boolean values.

@rquadling
Copy link
Contributor

rquadling commented Apr 22, 2022

11 years for a reply! Nice! (And thank you @cmb69 for checking!).

Under normal conditions, values read from the ini/config space are converted and held in a useful/normalised form (https://github.com/krakjoe/apcu/blob/eb28fe1ab0918e604bd22c6c1c58bae5280b7d72/php_apc.c#L129 is one example of processing ini entries).

If ini_get() returned that useful form ... then there's probably not a huge amount of negative to that.

Of course if people are using some_setting=not-false-not-zero-not-empty-so-must-be-true for a boolean type ... their "descriptive" value would be reduced to just true.

But this would also help show up issues like this:

https://www.php.net/manual/en/apcu.configuration.php#ini.apcu.slam-defense says the value is an int and is evaluated as a percentage.

But https://github.com/krakjoe/apcu/blob/eb28fe1ab0918e604bd22c6c1c58bae5280b7d72/php_apc.c#L141 clearly shows this is a boolean value.

I raised this as a possible issue in PHP-DI/PHP-DI#625

If nothing more, if phpinfo() reported supplied and interpreted/normalised values side by side then that would certainly be an improvement.

And how about if there was a --check-config option to PHP (similar to how you can check NGinx configs without actually waiting for the server to die on you when the config is incorrect/invalid) so that the config could be read, normalised and then cast back to the same type as it was supplied as ... and if the value is different, report that out.

So, supplying an integer for apc.slam_defense = 75 is converted to apc.slam_defense = true (as the type is a boolean really, no matter what the documentation says), and that is then cast back to an int as apc.slam_defence = 1. And, clearly, integer 1 !== integer 75 so php --check-config could/would report this as a potential failure.

@iluuu1994
Copy link
Member Author

iluuu1994 commented Jun 9, 2022

Given the feedback I'm closing this in favor of #8454 which might solve the original feature request (#7944) in a more generic way (ini_set(max(ini_parse_quantity(ini_get('memory_limit')), $upperBound))).

@iluuu1994 iluuu1994 closed this Jun 9, 2022
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.

api to set memory_limit to "at least" this much
7 participants