![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png){kind=logo}
Opened 16 months ago
Last modified 5 months ago
#63206 reviewing enhancement
WP_Filesystem and request_filesystem_credentials
| Reported by: | SirLouen | Owned by: | |
|---|---|---|---|
| Priority: | normal | Milestone: | Awaiting Review |
| Component: | General | Version: | |
| Severity: | normal | Keywords: | needs-patch needs-unit-tests |
| Cc: | Focuses: |
Description
I want to open this ticket as a follow-up for #62718. I was trying to build some Unit Tests for this, and I was banging my head trying to figure out a test that made sense because all the edge cases, felt too obvious.
But suddenly, I thought: "What if maybe WP_Filesystem() is not mutually exclusive with request_filesystem_credentials?"
So after doing a little research, I found out, that there are a total of 10 coincidences
- https://github.com/WordPress/wordpress-develop/blob/3998c85e880321877abb8105a5b2a97021745fbf/src/wp-admin/update-core.php#L881
- https://github.com/WordPress/wordpress-develop/blob/3998c85e880321877abb8105a5b2a97021745fbf/src/wp-includes/update.php#L1131
- https://github.com/WordPress/wordpress-develop/blob/3998c85e880321877abb8105a5b2a97021745fbf/src/wp-admin/includes/theme.php#L46
- https://github.com/WordPress/wordpress-develop/blob/3998c85e880321877abb8105a5b2a97021745fbf/src/wp-admin/includes/class-wp-upgrader.php#L244
- https://github.com/WordPress/wordpress-develop/blob/3998c85e880321877abb8105a5b2a97021745fbf/src/wp-admin/includes/class-wp-upgrader.php#L1019
- https://github.com/WordPress/wordpress-develop/blob/3998c85e880321877abb8105a5b2a97021745fbf/src/wp-admin/includes/class-wp-site-health.php#L1896
- https://github.com/WordPress/wordpress-develop/blob/3998c85e880321877abb8105a5b2a97021745fbf/src/wp-admin/includes/ajax-actions.php#L4411
- https://github.com/WordPress/wordpress-develop/blob/3998c85e880321877abb8105a5b2a97021745fbf/src/wp-admin/includes/ajax-actions.php#L4757
- https://github.com/WordPress/wordpress-develop/blob/3998c85e880321877abb8105a5b2a97021745fbf/src/wp-admin/includes/plugin.php#L932
9 of which actually use request_filesystem_credentials to grab and pass the credentials.
And just one that actually grab the credentials, but doesn't pass them.
https://github.com/WordPress/wordpress-develop/blob/3998c85e880321877abb8105a5b2a97021745fbf/src/wp-admin/includes/class-wp-site-health-auto-updates.php#L340
And something says that this code is only accounting for direct because right after the WP_Filesystem() call it will return a false if it's not direct
So my question here is: Why not introducing request_filesystem_credentials straight on WP_Filesystem function, and definitely and future-proof it, removing all errors like the one in #62718?
I think it's not intuitive the fact that WP_Filesystem is not actually adding request_filesystem_credentials by default, so unless you deal with this regularly, it's easy to miss.
The problem with this is that this brings a major code refactor touching a couple of files at once. I may first write some unit tests to cover this scenario and then add a patch for this.
Hello SirLouen,
I believe it would be great but is there any further step about this?
Replying to SirLouen: