#52342 closed enhancement (fixed)
Update copy_dir and _copy_dir for $dirlist error reporting
Reported by: | afragen | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 5.7 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Upgrade/Install | Keywords: | has-patch early commit |
Focuses: | Cc: |
Description (last modified by )
Previously discussed patches to copy_dir()
and _copy_dir()
to return a WP_Error reporting a failed directory listing. This should help with #51928 telemetry data.
This was originally in #51857 patch, but extracted here as #51857 is going in a different direction.
Discussed in #core https://wordpress.slack.com/archives/C02RQBWTW/p1611255822032100
Change History (14)
This ticket was mentioned in PR #908 on WordPress/wordpress-develop by afragen.
4 years ago
#1
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
4 years ago
This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.
4 years ago
#4
@
4 years ago
From scrub today, this ticket is ready for a review by @dd32. Hoping to get this one merged for 5.7 Beta 1 next week.
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
4 years ago
#6
@
4 years ago
Looking at PR908, is the $first_pass
parameter actually needed in practice?
Looking at all of the WP_Filesystem dirlist()
methods, there's three distinct return types: false
, array()
, and array( item, item )
; Error, no files, and files respectively.
Even if it was triggered in one of the recursive loops, I think a WP_Error
in that case would also be relevant here?
It feels like just checking for false
here would be enough?
The only questionable one is ftpext::dirlist() where I'm thinking there's a possibility that some FTP servers might not respect the -a
flag passed and not return .
entries.. but I think that's so edge case I'm just looking for a real reason to say it's problematic.
It looks like I changed it from being false === $list
in [12999] so I must've tested it at that point in time.
And yes, this needs to go in early in a cycle, and should not be backported to a point release.
#7
follow-up:
↓ 8
@
4 years ago
@dd32 the potential issue is that if the dirlist()
fails the first time through there is no error generated. This generates that error. Also, an error here means that the install_package()
failed and therefore the update failed leaving an empty plugin/theme folder.
Yes if it triggers during one of the loops that is sufficient.
The $first_pass
parameter is needed if there an issue with the initial dirlist()
only.
#8
in reply to:
↑ 7
@
4 years ago
Replying to afragen:
@dd32 the potential issue is that if the
dirlist()
fails the first time through there is no error generated.
Right, I'm pondering if this would be simple enough:
if ( false === $dirlist ) { return new WP_Error( 'dirlist_failed__copy_dir', __( 'Directory listing failed.' ), basename( $to ) ); }
An empty directory (array()
) wouldn't trigger that.
#9
@
4 years ago
The reason for the $first_pass was I think dirlist() could be false with a empty directory. If that’s the case an empty subfolder could cause a WP_Error. I didn’t want to risk some dev putting in an empty folder.
At least that was my initial logic. If I’m wrong then the check above is good.
I’ll defer to your experience and expertise.
Previously discussed patches to copy_dir() and _copy_dir() to return a WP_Error reporting a failed directory listing. This should help with $51928 telemetry data.
This was originally in #51857 patch, but extracted here as #51857 is going in a different direction.
Discussed in #core https://wordpress.slack.com/archives/C02RQBWTW/p1611255822032100
Trac ticket: https://core.trac.wordpress.org/ticket/52342