Fatal error in Site Health test_check_wp_filesystem_method when using other FS_METHOD than direct

[lakrisgubben]

The site health test WP_Site_Health_Auto_Updates::test_check_wp_filesystem_method generates a fatal error Call to undefined function submit_button() if the site is using another FS_METHOD than direct. The problem seems to be that the submit_button function is not available in a REST context.

To test this out add define( 'FS_METHOD', 'ftpext' ); to your wp-config.php file and then visit the site health page.

The issue can be fixed by adding a function_exists check for submit_button here: ​https://github.com/WordPress/wordpress-develop/blob/master/src/wp-admin/includes/class-wp-site-health-auto-updates.php#L280 something like this:

// Make sure the `submit_button` function is available during our REST call.
if ( ! function_exists( 'submit_button' ) ) {
        require_once ABSPATH . '/wp-admin/includes/template.php';
}

[SergeyBiryukov]

Hi there, welcome to WordPress Trac!

Thanks for the report, I was able to reproduce the issue as described.

Adds a function exists check for submit_button to make sure the site health tests do not trigger a fatal error when another FS_METHOD than direct is being used.

Trac ticket: https://core.trac.wordpress.org/ticket/53206

[lakrisgubben]

thanks @SergeyBiryukov! I've opened a ​PR on github with the fix mentioned above. :)

[SergeyBiryukov]

Thanks for the PR!

I wonder if it would be a bit more clear if this was directly before submit_button() is used in request_filesystem_credentials(). I don't have a strong preference so far though :)

[lakrisgubben]

I tried searching the WP codebase to see if there was any precedent for this kind of situation (where an admin function/file was required when not in admin and that function depended on another file only loaded in admin) but I couldn't find anything . Do you @SergeyBiryukov or someone else that has a deeper knowledge of the codebase than me know if any such precedent exists?

I can see merit in both ways of doing it. Keeping it like in the PR might be more clear since it's "closer" to where the issue arose, but moving it to the request_filesystem_credentials function will alleviate any issues going forward if that function would be used outside of an admin context in another place. No strong opinion about it though. :)

[mukesh27]

I also reproduce the issue as per the context.

Thanks for the PR. ​PR #1258 fix the issue for me.

[SergeyBiryukov]

Replying to lakrisgubben:

I tried searching the WP codebase to see if there was any precedent for this kind of situation (where an admin function/file was required when not in admin and that function depended on another file only loaded in admin) but I couldn't find anything . Do you @SergeyBiryukov or someone else that has a deeper knowledge of the codebase than me know if any such precedent exists?

Great question, I think some similar instances of function calls depending on files that may or may not be loaded would be the function_exists() checks for:

• request_filesystem_credentials() in WP_Site_Health_Auto_Updates::test_check_wp_filesystem_method()
• get_core_checksums() in WP_Site_Health_Auto_Updates::test_all_files_writable()
• got_mod_rewrite() in WP_Site_Health::get_test_authorization_header()
• get_post_states() in WP_Customize_Nav_Menus::customize_register()
• media_buttons() in _WP_Editors::editor()
• wp_die() in WP_Fatal_Error_Handler::display_default_error_template()
• wp_generate_password() in WP_Recovery_Mode_Cookie_Service::recovery_mode_hash()
• get_plugins() in WP_Recovery_Mode_Email_Service::get_plugin()
• wp_generate_password() in WP_Recovery_Mode_Link_Service::handle_begin_link()
• wp_generate_password() in WP_Recovery_Mode::handle_error()
• wp_safe_redirect() in WP_Recovery_Mode::redirect_protected()
• get_plugins() in wp_update_plugins()
• get_sample_permalink() in WP_REST_Posts_Controller::prepare_item_for_response()

At a glance, some of them are above the function call and some are not, but I'd say most of them are :) This way seems a bit clearer. Otherwise, for example, WP_Recovery_Mode_Link_Service::handle_begin_link()​ checks for the existence of wp_generate_password(), but it's not immediately clear that the function is eventually used in WP_Recovery_Mode_Cookie_Service::generate_cookie()​.

That said, when making this list I've noticed there's an existing check for request_filesystem_credentials() directly above the proposed code, which I missed earlier, so I think the PR is good as is for now :) Marking for commit.

I can see merit in both ways of doing it. Keeping it like in the PR might be more clear since it's "closer" to where the issue arose, but moving it to the request_filesystem_credentials function will alleviate any issues going forward if that function would be used outside of an admin context in another place. No strong opinion about it though. :)

Right. If we have another use case later for calling request_filesystem_credentials() outside of an admin context, we can always reconsider this :)

[lakrisgubben]

Sounds good, thanks for the warm welcome and helping out @SergeyBiryukov :)

[Clorith]

To me it makes more sense if the check for the function, and loading of dependencies, happened in request_filesystem_credentials. This would also make that function a bit more REST-friendly for other purposes without needing to load deep dependencies in "irrelevant" places.

[SergeyBiryukov]

In 50979:

Site Health: Make sure the submit_button() function is available in request_filesystem_credentials().

This avoids a fatal error when the function is called via REST API from WP_Site_Health_Auto_Updates::test_check_wp_filesystem_method().

Props lakrisgubben, mukesh27, Clorith, SergeyBiryukov.
Fixes #53206.