Only set WP_MAX_MEMORY_LIMIT by default when its greater than memory_limit

[danielbachhuber]

Even when memory_limit is set to 1GB or greater, WordPress will set WP_MAX_MEMORY_LIMIT to 256MB unless you've explicitly defined the constant. However, unless the developer is already aware of this, the constant can unintentionally break imports and other processes. I'd expect WordPress to respect the value set in memory_limit.

The constant was introduced in r17749

Previously:

• ​https://github.com/Automattic/vip-quickstart/issues/398
• ​https://twitter.com/nacin/status/575317779793985536

[jeremyfelt]

So if memory_limit < 256MB, set WP_MAX_MEMORY_LIMIT to 256MB, otherwise set WP_MAX_MEMORY_LIMIT to memory_limit?

[nacin]

Replying to jeremyfelt:

So if memory_limit < 256MB, set WP_MAX_MEMORY_LIMIT to 256MB, otherwise set WP_MAX_MEMORY_LIMIT to memory_limit?

Sure, that seems fine. I think I first thought we should just set WP_MAX_MEMORY_LIMIT to 256MB but then ignore its value, but that would cause problems with anyone manually using it. (And we use it a bunch of times, wrapping it with either the admin_memory_limit or image_memory_limit filters.

If someone is ambitious: I'd suggest a utility called wp_raise_memory_limit() that can only raise it. That would allow us to DRY up some things.

[A5hleyRich]

Implemented suggestions by @nacin. WP_MAX_MEMORY_LIMIT is set to memory_limit if greater than php.ini memory_limit. New function wp_raise_memory_limit() added which sets the limit and allows the value to be filtered (can only be raised).

[jrf]

There's a serious problem with the proposed patch as it doesn't take into account that the memory limit can be provided using shorthand notation such as '1G' or '256M'.

intval() will just cut off the G or M from the string which results in 1G < 256M and 1024K > 256M.

I will upload a refreshed patch which will take care of this.

Refs:
​http://php.net/manual/en/faq.using.php#faq.using.shorthandbytes
​http://php.net/manual/en/function.ini-get.php#refsect1-function.ini-get-examples

[jrf]

Ok, improved patch uploaded.

While working on it, I also found a bug in the setting of the 'normal' memory limit within wp_initial_constants().
Here the conversion only accounted for shorthand byte setting using M or G, but did not account for the possibility of the value having been set as a byte integer (or potentially as K).

The new patch also fixes that.

Other notes:

• Removed function_exists( 'memory_get_usage' ) for setting the 'normal' memory limit. This is no longer needed since PHP 5.2.1 and as that's below the WP minimum requirement, no longer needed here.
• Verifies that the new max limit is not below the current limit which could happen if a plugin would have changed the memory limit.
• Added documentation for the filter and references to where the documentation can be found.
• The wp_php_ini_bytes_to_int() unfortunately has to be added to the wp-includes/load.php file as it has to be available *before* the default constants are set in wp_initial_constants() which is done before wp-includes/functions.php is loaded.
• The patch does *not* use the new WP 4.4 constants for the byte conversion as the wp_php_ini_bytes_to_int() function is created before they are available.
• Added a minimal test case for phpunit. A more extensive test case would be desirable, however this can not be done as the test suite uses -1 for WP_MAX_MEMORY_LIMIT which will effectively bypass the negotiations which we need to test.

[jrf]

Grr.. Tabs, not spaces...

[SergeyBiryukov]

In the current patches, there's no indication that image_memory_limit filter was added in 3.5.0.

Instead of passing the filter name to the function and then using apply_filters() on a dynamic filter name, I'd suggest passing the context: wp_raise_memory_limit( 'admin' ) or wp_raise_memory_limit( 'image' ), then applying the corresponding filter depending on context. That would allow us to keep to original documentation for these filters, which I think is more clear.

[jrf]

@SergeyBiryukov Good point. Fixed in the newly uploaded 32075-improved-patch-v3.patch.

[jrf]

Updated docblocks

[markoheijnen]

With the current patch the argument $filter isn't set. That should become {$context}_memory_limit.

I do wonder if we can remove the whole switch block. It feels weird to only have it for the docblocks for the filters. Maybe @DrewAPicture has feedback on this. Also having a default would be great so people could use it for their own usage.

[jrf]

Fixed filter names

[jrf]

With the current patch the argument $filter isn't set. That should become {$context}_memory_limit.

Good catch. Was an artifact remaining from an earlier version which didn't have the switch. Fixed in v5 of the patch.

Also having a default would be great so people could use it for their own usage.

Not 100% sure what you mean, but the default for the $filtered_limit is the WP_MAX_MEMORY_LIMIT constant which can be set in wp-config.php and defaults to 256M or the php.ini memory limit, whichever is higher.

[markoheijnen]

Replying to jrf:

Not 100% sure what you mean, but the default for the $filtered_limit is the WP_MAX_MEMORY_LIMIT constant which can be set in wp-config.php and defaults to 256M or the php.ini memory limit, whichever is higher.

Personally I don't like constants but I do get your point. Say I use a plugin that imports CSV's and that plugin will use this new method wp_raise_memory_limit('csv'). I will be able to increase it with csv_memory_limit to 512M when needed.

[DrewAPicture]

@markoheijnen I think the switch block is fine, especially if new contexts ever get added. I also agree on adding a default, but if one of the two cases in the switch uses the default sdmin context, then perhaps the combine the default with the case.

@jrf In the DocBlocks, the summaries should use third-person singular verbs and should be a summary, rather than a title. So just remove "WP raise memory limit" and promote "Raises the PHP memory limit for memory intensive processes." to the top as the summary.

Also, if you pass --no-prefix when you generate your git diffs, it won't print out all your personal information in the patch ;-)

[jrf]

Added default case & updated docblocks

[jrf]

Added default case & updated docblocks

[jrf]

@markoheijnen @DrewAPicture

Ah, ok, I understand now what you meant with the default. Added a default case with a dynamic filter. I haven't combined it with the admin filter as then we'd get a similar issue with the filter documentation as @SergeyBiryukov pointed out before. This is a new dynamic filter and should be documented as such IMHO.

Also updated the docblock documentation based on @DrewAPicture's feedback. Hope I got it right.

Also, if you pass --no-prefix when you generate your git diffs, it won't print out all your personal information in the patch ;-)

Thanks for the heads-up. Unfortunately this is not an option in the git client I use (which, other than that, is awesome! (GitExtensions for those wondering))

[rasobar]

The WP_MAX_MEMORY_LIMIT variable should become a setting variable. Currently it is not listed in wp-config.php nor can an admin set it.
It defaults to 256M in wp-includes/default-constants.php and is overwritten with each WP update.

Background: At our hosting infrastructure, Suhosin is notifying, when a customer's web space allocates more than 256M, which is triggering a fail2ban and the admin is finally blocked. We do not want to change the rather high 256M limit for any php apps, rather we would like to set WP_MAX_MEMORY_LIMIT to 128M , but only once and not after each update.

Tt may not be exactly the scope with this issue, but should be thought together with a memory raise request parameter.

[ocean90]

Replying to rasobar:

Currently it is not listed in wp-config.php nor can an admin set it.

Which doesn't mean that you can't add it to wp-config.php. wp_initial_constants() includes a ! defined( 'WP_MAX_MEMORY_LIMIT' ) check before it gets defined with 256M. You can also use the admin_memory_limit and image_memory_limit filters.

[jrf]

What is still needed to move this ticket forward ?

[mensmaximus]

I would like to throw an other aspect into the discussion. How usefull can it be to set WP_MAX_MEMORY_LIMIT to 256M by default if the allocated memory ( ini_get( 'memory_limit' ) ) is less than 256M? What would happen if a plugin developer would rely on the constants value and this value exceeds the memory allocated by the server (and restricted by the hosting company)? Imho the WP_MAX_MEMORY_LIMIT constant should never report a larger amount of memory than the maximum allocatable memory.

[jrf]

@mensmaximus

How usefull can it be to set WP_MAX_MEMORY_LIMIT to 256M by default if the allocated memory ( ini_get( 'memory_limit' ) ) is less than 256M?

The PHP memory_limit ini value can be changed at runtime which is what this function tries to do.

What would happen if a plugin developer would rely on the constants value and this value exceeds the memory allocated by the server (and restricted by the hosting company)?

You can never rely on the changing at runtime to succeed. This function *attempts* to raise the memory limit, there is no guarantee that it will. ini_set() will return false if it fails to change the value.

As a plugin developer, you should only rely on the success of this if you have control of the server your plugin runs on. This will generally only be the case for one-off, custom plugins.

In all other circumstances: if you get a lot of complaints from users about fatal errors related to memory usage, trying to lower the memory usage of your plugin is the safer (and better) practice.

Imho the WP_MAX_MEMORY_LIMIT constant should never report a larger amount of memory than the maximum allocatable memory.

AFAIK there is no reliable way to find out the maximum allocatable memory on the server, so there is no way I can think of to take that into account when setting the constant, quite apart from the fact that the constant can be defined in wp-config.php outside of our control.

N.B.: There is a memory_get_peak_usage( true ) PHP function, but the output of this is AFAIK not reliable across all servers.

[mensmaximus]

Replying to jrf:

Imho the WP_MAX_MEMORY_LIMIT constant should never report a larger amount of memory than the maximum allocatable memory.

AFAIK there is no reliable way to find out the maximum allocatable memory on the server, so there is no way I can think of to take that into account when setting the constant, quite apart from the fact that the constant can be defined in wp-config.php outside of our control.

I should have written "allocated" instead of "allocatable". As for now WP_MAX_MEMORY_LIMIT is simply set to 256M by default. There is no verification whether it is really possible to set this value. Plugins like WooCommerce "report" this value in the backend on a status page and a user is told "the memory is ok".

You are absolutely correct a developer should never ever rely on any constant. But as you can see it is done. Not in the means of a "dependency" but in the means of reported "unchecked and unfiltered". Reporting something to the user that isn't true and may complicate the troubleshooting is simply spoken wrong.

Imho you have to get the allocated memory first, than try to increase it to 256M. If the return value is true you can define WP_MAX_MEMORY_LIMIT to 256M otherwise you set it to the currently allocated memory.

In today's hosting environments you will find PHP-FPM very often and there it is easy to restrict the memory allocation by defining it with php_admin_value[memory_limit] instead of php_value[memory_limit].

[jrf]

As for now WP_MAX_MEMORY_LIMIT is simply set to 256M by default. There is no verification whether it is really possible to set this value.

As the constant can be set in wp-config.php as well, this is - IMHO - not really in the scope of this issue. The WP_MAX_MEMORY_LIMIT is a *target* value, not a definite.

Plugins like WooCommerce "report" this value in the backend on a status page and a user is told "the memory is ok".

You are absolutely correct a developer should never ever rely on any constant. But as you can see it is done. Not in the means of a "dependency" but in the means of reported "unchecked and unfiltered". Reporting something to the user that isn't true and may complicate the troubleshooting is simply spoken wrong.

It's useful information if presented properly. The fact that WooCommerce apparently presents it with the presumption that the actual setting will always succeed is wrong. However, that is more of a plugin developer education issue which is again - IMHO - outside of the scope of this issue.

Imho you have to get the allocated memory first, than try to increase it to 256M. If the return value is true you can define WP_MAX_MEMORY_LIMIT to 256M otherwise you set it to the currently allocated memory.

The constant only provides the target value and - again - as this can also be set in wp-config.php, validation of whether the target value is realistic is not in the scope of this issue. This issue is about making sure it's never *lower* than the *current* memory limit.

The actual raising of the memory is only needed/done in WP in two particular circumstances at this moment and normally not needed. Doing so and then reverting on every page load just to try to get a more realistic target constant seems superfluous to me.

In today's hosting environments you will find PHP-FPM very often and there it is easy to restrict the memory allocation by defining it with php_admin_value[memory_limit] instead of php_value[memory_limit].

Good point and that is actually something I can work with, though it won't be fool-proof (re: setting of constant inwp-config.php). Let me see what I can come up with on that end.

[jrf]

Rebased against master, added check for changability of the memory limit, added unit tests

[jrf]

Added a check for whether the memory limit can actually be changed as per @mensmaximus suggestion.
This check is applied to the setting of both the WP_MEMORY_LIMIT constant as well as the WP_MAX_MEMORY_LIMIT constant.

This makes the value of these constants more realistic and prevents trying to change the memory limit if it cannot be changed.
The actual end result of the attempt to raise the memory is not affected by this, so it's more a cosmetic change than anything else.

The check does necessitate yet another new function to be added to the load.php file.

Other than that, the new patch (v7):

• has been rebased against the current master.
• adds unit tests for both of the new functions in load.php. As there was no test file for load.php yet (AFAICS), I've created one.
• contains a minor improvement in how the wp_php_ini_bytes_to_int() function deals with large values on 32-bit systems. Please note: This change does prevent the memory limit being raised to > 2G on 32-bit systems, however it prevents the value being disregarded completely.
• presumes that this patch will not make it into 4.5 (upped the @since tags to 4.6.0).
• adds @since changelog tags to the filters.
• fixes a typo in the documentation.

[jrf]

P.S.: and just in case someone sees the wp_is_ini_value_changable() function and now thinks:

"Hang on, that's wrong, INI_ALL and INI_USER aren't constants, aren't they called PHP_INI_ALL and PHP_INI_USER ?".

Sorry, but no, you're wrong ;-)

(Confused me briefly as well)
@see ​http://php.net/manual/en/info.constants.php

[jrf]

Can someone either tell me what else needs doing to get this mergable or please merge this in ?

[jrf]

Minor tidying up

[jrf]

Rebased against current codebase

[swissspidy]

Thanks again for bringing this ticket up, @jrf!

Me and @ocean90 both had a look at it and think it's fine.

In 32075.4.diff​ I only made a few adjustments.

• Various docs improvements
• Make wp_raise_memory_limit() the value that was attempted to set or false on failure
• Make some parts a bit more readable
• Fix spelling of wp_is_ini_value_changeable()

[ocean90]

32075.5.diff​:

• Renames wp_php_ini_bytes_to_int() to wp_convert_php_ini_bytes_to_int() to match wp_convert_hr_to_bytes().
• Uses 256 * MB_IN_BYTES.

[ocean90]

Looking at it again, wp_convert_hr_to_bytes() and wp_convert_php_ini_bytes_to_int() are doing the same, except for the PHP_INT_MAX handling.

[ocean90]

Use wp_convert_hr_to_bytes()

[ocean90]

@jrf Any thoughts on 32075.6.diff​?

[jrf]

@ocean90 I haven't had the time to have a good look yet - will get back to you by Wednesday at the latest if that's ok.

[jrf]

@ocean90 @swissspidy Thanks for working with me on this.

I've had a good look at the latest iterations.

• Generally: looks good.
• Very good catch on the wp_convert_hr_to_bytes() function ! Completely missed that one (and I did actually search for something like it). However, the docblock of the wp_convert_hr_to_bytes() function is less informative than the new docblock was, so I think it would be good to keep some of the documentation improvements.
• As the BYTE constant definitions in default-constants.php have been moved up to before the first use of wp_convert_hr_to_bytes(), that function can now use the constant KB_IN_BYTES.
• Personally, I find that the readability of the code had decreased in the few places in the patch where a conditional setting a value to be returned was replaced by having the condition in the return statement. This does not make the code faster and can more easily lead to bugs.
• The 256 * MB_IN_BYTES is an unnecessary calculation, the end value will never change, so it should be a static value. If this change was made for clarity a // = 256Mb comment would be more appropriate.

[Edit]
Oh and one more thing: the return value can actually be set with more precision as we can check whether the ini_set() succeeded.
That way false is returned if nothing was changed and the new limit which was set will be returned if all is well.

And another: spelling change to changeable has not been consistently applied everywhere.

Last thing out of curiosity - I've been racking my brain what the hr in the wp_convert_hr_to_bytes() function name stands for... anyone care to enlighten me ? Might be helpful to have as part of the documentation as well.

[jrf]

Changes:

• Reverted, but documented 256 * MB_IN_BYTES.
• Improved documentation of the $context parameter for the wp_raise_memory_limit() function - added information about possibility to use arbitrary context.
• Improved return value of wp_raise_memory_limit(). Returns false if memory was not changed and new value if it was.
• Carried over some documentation improvements which had been applied in the mean time to moved round code.
• Minor improvement to the image_memory_limit filter documentation - precision of what passed parameter is.
• Merged documentation of the wp_convert_hr_to_bytes() function with the previous documentation for wp_convert_php_ini_bytes_to_int().
• Use the GB/MB/KB_IN_BYTES constants in wp_convert_hr_to_bytes() as they are now available before the function is first used.
• Minor documentation and spelling improvements in the new unit test file.

[jrf]

Grr... fixed double line spacing x 2

[ocean90]

In 38012:

Boostrap: Move wp_convert_hr_to_bytes() to wp-includes/load.php.

wp_convert_hr_to_bytes() was previously defined in wp-includes/media.php because it's only used by wp_max_upload_size() in the same file.
Moving this function to load.php allows us to improve core's memory limit handling.

See #32075.

[ocean90]

In 38013:

Bootstrap: Clean up wp_convert_hr_to_bytes().

• Don't return a value higher than PHP_INT_MAX.
• Add unit tests.

Props jrf.
See #32075.

[ocean90]

Replying to ocean90:

In 38013:

Bootstrap: Clean up wp_convert_hr_to_bytes().

• Don't return a value higher than PHP_INT_MAX.
• Add unit tests.

Props jrf.
See #32075.

Note: I didn't change it to use substr() because it was between 5 and 7 times slower than the current implementation.

[ocean90]

In 38015:

Bootstrap: Enhance core's memory limit handling.

• Don't lower memory limit if the current limit is greater than WP_MAX_MEMORY_LIMIT.
• Set WP_MEMORY_LIMIT and WP_MAX_MEMORY_LIMIT to current limit if the memory_limit setting can't be changed at runtime.
• Use wp_convert_hr_to_bytes() when parsing the value of the memory_limit setting because it can be a shorthand or an integer value.
• Introduce wp_raise_memory_limit( $context ) to raise the PHP memory limit for memory intensive processes. This DRYs up some logic and includes the existing admin_memory_limit and image_memory_limit filters. The function can also be used for custom contexts, the {$context}_memory_limit filter allows to customize the limit.
• Introduce wp_is_ini_value_changeable( $setting ) to determine whether a PHP ini value is changeable at runtime.
• Remove a function_exists( 'memory_get_usage' ) check. Since PHP 5.2.1 support for memory limit is always enabled.

Related commits: [38011-38013]

Props jrf, A5hleyRich, swissspidy, ocean90.
Fixes #32075.

[ocean90]

In 38016:

Unit tests: Don't change the memory_limit setting during tests.

40M isn't enough and can lead to an "out of memory" error. Change test_wp_raise_memory_limit() to test that wp_raise_memory_limit() doesn't *lower* the memory limit.

See [38015].
See #32075.

[ocean90]

In 38017:

Bootstrap: Make wp_is_ini_value_changeable() compatible with PHP 5.2.6 - 5.2.17.

There is a bug in PHP 5.2.6 - 5.2.17 (​https://bugs.php.net/bug.php?id=44936, ​https://3v4l.org/IL0A2) which changes the access level of a setting to 63 after ini_set() was called.
To continue comparing the access value against INI_ALL and INI_USER use the bit operator & 7:

• 1 & 7 === 1 (INI_USER)
• 2 & 7 === 2 (INI_PERDIR)
• 4 & 7 === 4 (INI_SYSTEM)
• 7 & 7 === 7 (INI_ALL)
• 63 & 7 === 7 (INI_ALL)

See [38015].
See #32075.