WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 7 weeks ago

#49329 new defect (bug)

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

Reported by: espiat Owned by:
Milestone: 5.5 Priority: normal
Severity: normal Version: 5.3.2
Component: Site Health Keywords: has-patch 2nd-opinion
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 8 weeks ago.
49329.2.diff (1.7 KB) - added by Clorith 7 weeks ago.

Download all attachments as: .zip

Change History (12)

#1 @espiat
2 months 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
2 months ago

  • Keywords reporter-feedback removed

#3 @SergeyBiryukov
8 weeks ago

  • Milestone changed from Awaiting Review to 5.4

@SergeyBiryukov
8 weeks ago

#4 @SergeyBiryukov
8 weeks 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.


8 weeks ago

#6 follow-up: @Clorith
7 weeks 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
7 weeks ago

#7 @Clorith
7 weeks 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
7 weeks 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
7 weeks 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
7 weeks 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)

Note: See TracTickets for help on using tickets.