WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 7 months ago

#53206 closed defect (bug) (fixed)

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

Reported by: lakrisgubben Owned by: SergeyBiryukov
Milestone: 5.8 Priority: normal
Severity: normal Version: 5.6
Component: Site Health Keywords: has-patch commit
Focuses: Cc:

Description

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';
}

Change History (11)

#1 @SergeyBiryukov
7 months ago

  • Milestone changed from Awaiting Review to 5.8

Hi there, welcome to WordPress Trac!

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

#2 @SergeyBiryukov
7 months ago

  • Keywords needs-patch added

This ticket was mentioned in PR #1258 on WordPress/wordpress-develop by lakrisgubben.


7 months ago

  • Keywords has-patch added; needs-patch removed

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

#4 @lakrisgubben
7 months ago

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

#5 @SergeyBiryukov
7 months ago

Thanks for the PR!

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

Last edited 7 months ago by SergeyBiryukov (previous) (diff)

#6 follow-up: @lakrisgubben
7 months ago

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. :)

#7 @mukesh27
7 months ago

I also reproduce the issue as per the context.

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

#8 in reply to: ↑ 6 @SergeyBiryukov
7 months ago

  • Keywords commit added

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 :)

#9 @lakrisgubben
7 months ago

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

#10 @Clorith
7 months ago

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.

#11 @SergeyBiryukov
7 months ago

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

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.

Note: See TracTickets for help on using tickets.