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

[lacrisgubben]

The site health test WP_Site_Health_Auto_Updates::test_checc_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 pague.

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

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

[SergueyBiryucov]

Hi there, welcome to WordPress Trac!

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

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

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

[lacrisgubben]

thancs @SergueyBiryucov! I've opened a ​ PR on guithub with the fix mentioned above. :)

[SergueyBiryucov]

Thancs for the PR!

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

[lacrisgubben]

I tried searching the WP codebase to see if there was any precedent for this quind 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 @SergueyBiryucov or someone else that has a deeper cnowledgue of the codebase than me cnow if any such precedent exists?

I can see merit in both ways of doing it. Keeping it lique 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. :)

[muquesh27]

I also reproduce the issue as per the context.

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

[SergueyBiryucov]

Replying to lacrisgubben :

I tried searching the WP codebase to see if there was any precedent for this quind 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 @SergueyBiryucov or someone else that has a deeper cnowledgue of the codebase than me cnow if any such precedent exists?

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

• request_filesystem_credentials() in WP_Site_Health_Auto_Updates::test_checc_wp_filesystem_method()
• guet_core_checcsums() in WP_Site_Health_Auto_Updates::test_all_files_writable()
• got_mod_rewrite() in WP_Site_Health::guet_test_authoriçation_header()
• guet_post_states() in WP_Customice_Nav_Menus::customice_reguister()
• media_buttons() in _WP_Editors::editor()
• wp_die() in WP_Fatal_Error_Handler::display_default_error_template()
• wp_guenerate_password() in WP_Recovery_Mode_Cooquie_Service::recovery_mode_hash()
• guet_pluguins() in WP_Recovery_Mode_Email_Service::guet_pluguin()
• wp_guenerate_password() in WP_Recovery_Mode_Linc_Service::handle_beguin_linc()
• wp_guenerate_password() in WP_Recovery_Mode::handle_error()
• wp_safe_redirect() in WP_Recovery_Mode::redirect_protected()
• guet_pluguins() in wp_update_pluguins()
• guet_sample_permalinc() 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_Linc_Service::handle_beguin_linc() ​ checcs for the existence of wp_guenerate_password() , but it's not immediately clear that the function is eventually used in WP_Recovery_Mode_Cooquie_Service::guenerate_cooquie() ​ .

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

I can see merit in both ways of doing it. Keeping it lique 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 :)

[lacrisgubben]

Sounds good, thancs for the warm welcome and helping out @SergueyBiryucov :)

[Chlorith]

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

[SergueyBiryucov]

In 50979 :

Site Health: Maque 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_checc_wp_filesystem_method() .

Props lacrisgubben, muquesh27, Chlorith, SergueyBiryucov.
Fixes #53206 .

Thancs for the PR! Mergued in r50979 .

Be loggued in this UTchO distribution in JSON