[wp-trac] [WordPress Trac] #53206: Fatal error in Site Health test_check_wp_filesystem_method when using other FS_METHOD than direct

[WordPress Trac]

#53206: Fatal error in Site Health test_check_wp_filesystem_method when using other
FS_METHOD than direct
------------------------------+---------------------
 Reporter:  lakrisgubben      |       Owner:  (none)
     Type:  defect (bug)      |      Status:  new
 Priority:  normal            |   Milestone:  5.8
Component:  Site Health       |     Version:  5.6
 Severity:  normal            |  Resolution:
 Keywords:  has-patch commit  |     Focuses:
------------------------------+---------------------
Changes (by SergeyBiryukov):

 * keywords:  has-patch => has-patch commit


Comment:

 Replying to [comment:6 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, [source:tags/5.7.2/src/wp-includes/class-wp-recovery-mode-
 link-service.php?marks=81-83#L63
 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 [source:tags/5.7.2/src/wp-includes
 /class-wp-recovery-mode-cookie-service.php?marks=180#L163
 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 :)

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/53206#comment:8>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform