Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#49329 closed defect (bug) (fixed)

memory_limit in site health is not really correct, value is taken from wp_raise_memory_limit

Reported by: espiat's profile espiat Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.5 Priority: normal
Severity: normal Version: 5.3.2
Component: Site Health Keywords:
Focuses: administration Cc:

Description

Hi.

this is my first ticket.

Somebody saw that the memory_limit in the site health is not the value from the php.ini

So I started looking for it.

Problem:
The reason is the function wp_raise_memory_limit

This is called here:
https://github.com/WordPress/WordPress/blob/16b8d91baa830fd88eed7d6312477e4f0891798c/wp-admin/admin.php?fbclid=IwAR2pQTBHMaEZAvm1xpXSgfgBDh_ubgYrkMhGtDbFibs9Gom7CIha2kLF3-U#L156

And defined here:
https://github.com/WordPress/WordPress/blob/001ffe81fbec4438a9f594f330e18103d21fbcd7/wp-includes/functions.php#L6784

So if you are logged in as admin, the memory_limit will be raised to 256M, if this can be changed and the value in wp-config.php is not higher.

The value is therefore not specified correctly in Site Health, because it may have a different value in the frontend and for other users (apart from admin).

The Plugin Query Monitor also shows this value with a trick (more: https://github.com/johnbillion/query-monitor/blob/49485f9359cc57efdc3f4d2d86d342a43e6b6dcf/README.md#a-note-on-query-monitors-implementation)

Then I tried in the function (wp_raise_memory_limit) with if queries

'site-health' === $ screen-> id
or
$ site_health_screen = get_current_screen ()
to keep the real memory_limit

But that didn't work.

The raise function is called so early, that it is hard to get the real memory_limit.

my Solution:
(i currently doent know how to add "real" commit changes here in this trac system)

Adding the global var and call the value first in the wp_raise_memory_limit function (https://github.com/WordPress/WordPress/blob/001ffe81fbec4438a9f594f330e18103d21fbcd7/wp-includes/functions.php#L6784)

<?php

function wp_raise_memory_limit( $context = 'admin'  ) {

        global $server_memory_limit;
        $server_memory_limit = ini_get( 'memory_limit' );


        // Exit early if the limit cannot be changed.
...

Add the global var in the function 'debug_data()' (line:
https://github.com/WordPress/WordPress/blob/001ffe81fbec4438a9f594f330e18103d21fbcd7/wp-admin/includes/class-wp-debug-data.php#L32'

<?php
static function debug_data() {
                global $wpdb;
                global $server_memory_limit;

After including the global server memory we can return an extra row in the site health table (https://github.com/WordPress/WordPress/blob/001ffe81fbec4438a9f594f330e18103d21fbcd7/wp-admin/includes/class-wp-debug-data.php#L650):

<?php
$info['wp-server']['fields']['memory_limit_server']        = array(
                                'label' => __( 'PHP memory limit (server setting)' ),
                                'value' => $server_memory_limit,
                        );

That's it.

Thanks in advance for the feedback

Attachments (2)

49329.diff (1.4 KB) - added by SergeyBiryukov 5 years ago.
49329.2.diff (1.7 KB) - added by Clorith 5 years ago.

Download all attachments as: .zip

Change History (43)

#1 @espiat
5 years ago

I would also like to mention that all plugins (woocommerce, ...) fall back on this incorrect value in their debug information.

The plugins should also be able to access the real server value in the debug report or the value is also displayed in the site health.

#2 @zodiac1978
5 years ago

  • Keywords reporter-feedback removed

#3 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 5.4

@SergeyBiryukov
5 years ago

#4 @SergeyBiryukov
5 years ago

  • Keywords has-patch added; dev-feedback removed

Hi there, welcome to WordPress Trac! Thanks for the report, it's a good catch.

Just noting that as of [47063], WP_Site_Health is instantiated in wp-settings.php, earlier than wp_raise_memory_limit( 'admin' ) runs, so we can store the value in a class property instead of a global.

49329.diff seems to work as expected in my testing.

This ticket was mentioned in Slack in #core-site-health by sergey. View the logs.


5 years ago

#6 follow-up: @Clorith
5 years ago

As the memory limit for wp-admin may be important to some plugins (that do heavy processing in the admin area, and not the front end), it would make sense to show both of the values if they were to differ.

@Clorith
5 years ago

#7 @Clorith
5 years ago

  • Keywords 2nd-opinion added

49329.2.diff is my take on this, it adds an extra entry if the memory limit has been modified from the base value.

This prevents it from being listed if it's not needed, and also makes it clearer that there's a distinction, and what that distinction is.

I'm not entirely sure on the wording in the parenthesis of only for admin screens, I wanted ot avoid complicated terms like "Dashboard", and "wp-admin", since they may not always be easy to translate, and wp-admin it self may have been modified to be something else, which would lose the reference for the end user.

#8 @Clorith
5 years ago

  • Milestone changed from 5.4 to 5.5

Moving this to the 5.5 milestone for now, as we're starting preparations for the 5.4-beta period shortly.

#9 in reply to: ↑ 6 @zodiac1978
5 years ago

Replying to Clorith:

As the memory limit for wp-admin may be important to some plugins (that do heavy processing in the admin area, and not the front end), it would make sense to show both of the values if they were to differ.

I don't think this is the best way here. I'm on a hoster with 128M PHP memory limit. In Site Health it shows at the moment as 256M, because of this bug. Unfortunately you can *set* this higher limit without any errors, although the available memory is still 128M.

WordPress tries to set this to 256M, which is okay, because if it is available, then we can use 256M in the backend. But we shouldn't shows this value as it is not reliable.

I would recommend to go with the patch from @SergeyBiryukov and use this for WP 5.4.

If we find a way to show a reliable way to show the existing memory after trying the raise to 256M we could add this in 5.5.

@Clorith What do you think? Couldn't we better fix this now and iterate later?

#10 @espiat
5 years ago

In my opinion we should always display this original value because it is important for debugging. The site health values ​​are just there to show them.

I think we should call the labels with something like this - to make it clear:

PHP memory limit (Value may differ due to the following function: LINK) (LINK - https://developer.wordpress.org/reference/functions/wp_raise_memory_limit/)

Origin PHP memory limit (Server value)

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


5 years ago

#13 @SergeyBiryukov
5 years ago

  • Keywords commit added; 2nd-opinion removed

Per the latest chats in #core-site-health (linked above), 49329.2.diff seems to be the way to go. Marking for commit.

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


5 years ago

#15 @SergeyBiryukov
5 years ago

I think there might be some confusion with 49329.2.diff as it lists the original value as "(only for admin screens)", but the actual value used for admin screens doesn't have any remark:

PHP memory limit256M
PHP memory limit (only for admin screens)64M

I think it should be vice versa, something like:

PHP memory limit64M
PHP memory limit (only for admin screens)256M

And if both values are the same, just list it without a remark:

PHP memory limit256M
Last edited 5 years ago by SergeyBiryukov (previous) (diff)

#16 @SergeyBiryukov
5 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 47762:

Site Health: Display the original PHP memory limit on Site Health Info screen.

This ensures that if the limit has been modified for admin screens by wp_raise_memory_limit(), the original value is displayed along with the current value.

Props Clorith, espiat, zodiac1978, SergeyBiryukov.
Fixes #49329.

#17 follow-ups: @zodiac1978
5 years ago

  • Keywords needs-patch added; has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

This is not fixing the underlying problem.

I'm on a hoster with 128M PHP memory limit. In Site Health it shows at the moment as 256M, because of this bug. Unfortunately you can *set* this higher limit without any errors, although the available memory is still 128M.

Although my *real* memory limit is 128M, WordPress will show as the second value now *always* as 256M. No matter what the server is having as a real limit. This is confusing IMHO.

To my understanding this value is something like a "max-width" for CSS. Do not use more than 256M. It is not saying that this value is usable.

PHP-Speicher-Limit (memory_limit)	100M
PHP memory limit (only for admin screens)	256M

Maybe this is intentional. But I think it is misleading. Especially for people not knowing the exact definition of memory_limit. If it is showing 256M in every case. It is not serving any useful purpose I can think of.

#18 in reply to: ↑ 17 @SergeyBiryukov
5 years ago

Replying to zodiac1978:

I'm on a hoster with 128M PHP memory limit. In Site Health it shows at the moment as 256M, because of this bug. Unfortunately you can *set* this higher limit without any errors, although the available memory is still 128M.

Although my *real* memory limit is 128M, WordPress will show as the second value now *always* as 256M. No matter what the server is having as a real limit. This is confusing IMHO.

Thanks for bringing that up! That seems weird though, I don't think ini_set( 'memory_limit', '256M' ) is supposed to succeed if the value is not really changeable.

On most environments, ini_get( 'memory_limit' ) would return the correct memory limit. If it returns incorrect value in some particular environments, then it looks like there's no way for us to get the correct one?

Displaying just the original value would also be misleading, because that's not the current value for admin screens for most environments.

#19 in reply to: ↑ 17 @SergeyBiryukov
5 years ago

Replying to zodiac1978:

I'm on a hoster with 128M PHP memory limit. In Site Health it shows at the moment as 256M, because of this bug. Unfortunately you can *set* this higher limit without any errors, although the available memory is still 128M.

Although my *real* memory limit is 128M, WordPress will show as the second value now *always* as 256M. No matter what the server is having as a real limit. This is confusing IMHO.

Could you see what posix_getrlimit( 'totalmem' ) returns in that environment?

#20 @zodiac1978
5 years ago

I have two hoster with 128M real memory limit.

The first is showing 524288000 and the second one is showing unlimited for soft and hard totalmem. So both values are incorrect in the matter of the current available memory.

With ini_get( 'memory_limit' ); I get the correct value of 128M in both cases (if not in WP environment).

The problem is: I can use ini_set( 'memory_limit', '256M' ); and then ini_get( 'memory_limit' ); shows 256M (instead of the correct 128M).

#21 in reply to: ↑ 17 @zodiac1978
5 years ago

Replying to zodiac1978:

PHP-Speicher-Limit (memory_limit)	100M
PHP memory limit (only for admin screens)	256M

Additionally site health is showing (on 5.5-alpha-47814) the above at the moment which isn't true at all. I have 128M available, so neither 100M or 256M is true.

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


5 years ago

This ticket was mentioned in Slack in #hosting-community by javier. View the logs.


5 years ago

#24 follow-up: @JavierCasares
5 years ago

  • Keywords has-patch needs-testing 2nd-opinion added; needs-patch removed

Checking the @zodiac1978 comments...

The machine has 128M real memory limit, but PHP doesn't check the hardware configuration, it uses its own configuration... so, if a machine has 128MB RAM, you can configure 512MB for PHP. So, its possible that the machine crash, but the configuration is correct, because PHP has a 512MB configuration (not recommended, of course).

I can use ini_set( 'memory_limit', '256M' ); and then ini_get( 'memory_limit' ); shows 256M (instead of the correct 128M).

This is how PHP works... so, you can update the PHP config via ini_set(), and when you do ini_get() you will retrieve the new info, although the hardware has whatever...

I don't think this is an issue, is how PHP works.

#25 in reply to: ↑ 24 @zodiac1978
5 years ago

  • Keywords needs-patch added; has-patch removed

Replying to JavierCasares:

I can use ini_set( 'memory_limit', '256M' ); and then ini_get( 'memory_limit' ); shows 256M (instead of the correct 128M).

This is how PHP works... so, you can update the PHP config via ini_set(), and when you do ini_get() you will retrieve the new info, although the hardware has whatever...

I don't think this is an issue, is how PHP works.

Yes, but this is not the question here @JavierCasares

If I can raise the limit to any value without knowing if this is actually available and WP raises the value to 256M no matter what in WP admin environment we do not provide any useful debugging information. The value will always be 256M.

This means IMHO that the current patch is not useful and we should grab the memory_limit early and do not show the WP admin value, because it is not showing the actual available memory limit.

This ticket was mentioned in Slack in #hosting-community by mike. View the logs.


5 years ago

This ticket was mentioned in Slack in #hosting-community by jadonn. View the logs.


5 years ago

#28 follow-up: @kirasong
5 years ago

My gut instinct on this is that it's up to the server admin / syseng to make sure that the PHP limit is not set higher than the amount of physical RAM.

@zodiac1978 Is this an issue that you've seen trending? I'd be interested to find out more about the user concerns to see what the best way to solve for it would be.

A couple notes:

  • On shared systems, hardware memory being available doesn't necessarily mean it's available to the individual user or WordPress install/PHP.
  • Memory/RAM may report differently/shift while WordPress is being used if it is run across more than one machine (and probably more than one instance/container depending on how it's set up or how it's retrieved).

#29 in reply to: ↑ 28 @zodiac1978
5 years ago

Replying to mikeschroder:

My gut instinct on this is that it's up to the server admin / syseng to make sure that the PHP limit is not set higher than the amount of physical RAM.

@zodiac1978 Is this an issue that you've seen trending? I'd be interested to find out more about the user concerns to see what the best way to solve for it would be.

At the moment WordPress is showing 256M in Health Check because wp_raise_memory_limit has tried to set 256M. This is not failing on all hosters I know (even if just 128M is available on this shared hosting environment).

I disagree with @Clorith on this one:

As the memory limit for wp-admin may be important to some plugins (that do heavy processing in the admin area, and not the front end), it would make sense to show both of the values if they were to differ.

If we split those values in a frontend value and an admin value, the admin value will *always* show 256M, because it is not failing to be set.

Unfortunately this thought from @SergeyBiryukov is not true:

That seems weird though, I don't think ini_set( 'memory_limit', '256M' ) is supposed to succeed if the value is not really changeable.

If I look in Site Health and see a "memory_limit" info, then I expect this to be the true available limit. In my case it shows 100M and 256M (admin) which is both incorrect.

And as I said above it will show 256M for admin in every case. So what information do you gain if you are checking this value in Health Check? You just see what WP is *trying* to get.

I think the solution is to get the value with ini_get('memory_limit') as early as possible to get not affected from any plugins or WP which are trying to set something else and show only this value.

The only other reliable way to get the real value I know of is to try to allocate the memory and see when it fails. This is of course a risky way, so we decided against it:
https://github.com/WordPress/health-check/issues/50

Last edited 5 years ago by zodiac1978 (previous) (diff)

#30 follow-up: @JavierCasares
5 years ago

I understand the problem, but I think is a terminology one... We are showing the "PHP memory limit", so, we show the information PHP is telling us... we cannot check the hardware information (at least, easily from a simple PHP query)...

So, if we show "PHP memory limit" and we get the information from the ini_get, this should be real. If a plugin modifies the information, is also real, because is what PHP uses. If the user, hosting, or plugin do a bad configuration (setting more memory that the machine has) is something we cannot test (at least, as I said, not easily from the WordPress perspective).

We can check the "PHP initial memory limit" and "PHP memory limit to use" (or whatever), as the original ticket said, but I don't think is a good idea to stop this because we don't have access to the real memory.

#31 in reply to: ↑ 30 @zodiac1978
5 years ago

Replying to JavierCasares:

So, if we show "PHP memory limit" and we get the information from the ini_get, this should be real. If a plugin modifies the information, is also real, because is what PHP uses. If the user, hosting, or plugin do a bad configuration (setting more memory that the machine has) is something we cannot test (at least, as I said, not easily from the WordPress perspective).

If I write "real", I mean the value which is available per script to allocate. In my case 128M. If Site Health shows 100M and 256M this is a bug IMHO. This error (at least the last value about the admin limit) is coming from WordPress itself through wp_raise_memory_limit. About the plugins: This is why I recommend getting the limit as early as possible.

Showing the limit of 256M is confusing because I will get a Fatal Error after 128M (which is my real limit).

We can check the "PHP initial memory limit" and "PHP memory limit to use" (or whatever), as the original ticket said, but I don't think is a good idea to stop this because we don't have access to the real memory.

The two values are "initial memory limit" and "memory limit in the admin area". And I think the value doesn't make sense at all if it shows 256M no matter what. Please tell me how this is useful for debugging issues if we cannot tell if this value could be used? I don't get it.

#32 @zodiac1978
5 years ago

After we raised the PHP limit to 5.6 we could use ini_get_all (https://www.php.net/manual/de/function.ini-get-all.php) here, because with PHP 5.3 came the DETAILS parameter and we get a global value and a local value.

When details is TRUE (default) the array will contain global_value (set in php.ini), local_value (perhaps set with ini_set() or .htaccess), and access (the access level).

$ini_all[ 'memory_limit' ][ 'global_value' ] is 128M in my case, which is correct.

$ini_all[ 'memory_limit' ][ 'local_value' ] is whatever the user / plugin or WordPress tried to use. In the WP admin context this should be 256M (or higher if available).

There is a workaround for PHP 5.2 which could be removed now I think:
https://github.com/WordPress/WordPress/blob/26bda18a23174afb048afbe62296c76a62add542/wp-includes/load.php#L1330-L1333

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


4 years ago

#34 follow-up: @Clorith
4 years ago

I'm not sure I understand the problem still...

A host which does not have the memory to allocate could disable the function that lets PHP, and thus also WP, from changing the available memory limit.

I may need someone to summarize what the remaining issue in this ticket is, as I fear I'm not quite grasping it at this time.

#35 in reply to: ↑ 34 @zodiac1978
4 years ago

Replying to Clorith:

A host which does not have the memory to allocate could disable the function that lets PHP, and thus also WP, from changing the available memory limit.

I have not seen any hoster doing this. As I wrote above, you can set a higher limit than the actual available limit on the host for all hosters I know. Therefore the values are not reliable if not got very early in the chain or through the correct function.

I may need someone to summarize what the remaining issue in this ticket is, as I fear I'm not quite grasping it at this time.

In the latest alpha (5.5-alpha-48171) the values for me are:

PHP-Speicher-Limit (memory_limit)	100M
PHP memory limit (only for admin screens)	256M

Although I have 128M as the "real" limit (on this shared hoster). That is the actual bug at the moment.

256M are not available. WP is just trying to get this value. So this value is not helpful for my debugging IMHO.

100M is not the correct limit too. (No plugin activated, Twenty Twenty theme, no .htaccess or php.ini/.user.ini with different settings involved)

$ini_all[ 'memory_limit' ][ 'global_value' ] seems to be the better approach here IMHO.

We could use it if it is available and fallback to ini_get if not. Or we do it the hard way:

For example, doing a loopback request in which you add a shutdown handler for reporting and then fill up memory (while disabling error reporting!).

This was a suggestion from @schlessera on Twitter.

I think reporting the local value is not useful at all and should be removed again, because it is not reliable that the value of memory is really available. It is more like a "max-width" value for the memory which is not helpful is most cases.

#36 @SergeyBiryukov
4 years ago

  • Milestone changed from 5.5 to 5.5.1

This still needs a patch to account for ini_get( 'memory_limit' ) reporting incorrect values in some environments.

Seems like $ini_all['memory_limit']['global_value'] could be used instead. Let's follow up in 5.5.1.

#37 @khag7
4 years ago

So we're juggling 3 values here:

  1. the initial php memory limit
  2. the raised memory limit (which might be higher than what's even physically possible)
  3. the actual available memory limit

On one hand, it will be confusing to most users to show 3 values.
On the other hand, information is beneficial. A "power user" might want to see all 3.

I think showing $ini_all['memory_limit']['global_value'] in site health is a good idea. Whether or not the initial and raised memory limits are also shown is probably not useful to the average user but maybe some people would like to see those as well. Who is the target audience for this?

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by desrosj. View the logs.


4 years ago

#40 @desrosj
4 years ago

  • Milestone changed from 5.5.1 to 5.5.2

This still needs a refresh and more testing. With 5.5.1 now being a quicker release, punting this one until it can receive proper attention.

Last edited 4 years ago by desrosj (previous) (diff)

#41 @SergeyBiryukov
4 years ago

  • Keywords needs-testing 2nd-opinion needs-patch removed
  • Milestone changed from 5.5.2 to 5.5
  • Resolution set to fixed
  • Status changed from reopened to closed

Going to re-close this one as fixed in 5.5. Created #51153 to address the remaining issue.

Note: See TracTickets for help on using tickets.