WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 8 months ago

Last modified 4 months ago

#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 pbiron)

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.


8 months ago

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

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


8 months ago

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


8 months ago

#4 @hellofromTonya
8 months 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.


8 months ago

#6 @dd32
8 months 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: @afragen
8 months ago

@dd32 the potential issue is that if the dirlist() fails the first time through there is no error generated. This generates that error.

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.

Version 0, edited 8 months ago by afragen (next)

#8 in reply to: ↑ 7 @dd32
8 months 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 @afragen
8 months 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.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


8 months ago

#11 @afragen
8 months ago

  • Keywords commit added

PR updated per @dd32's input.

This ticket was mentioned in Slack in #core-auto-updates by hellofromtonya. View the logs.


8 months ago

#13 @SergeyBiryukov
8 months ago

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

In 50149:

Upgrade/Install: Return a WP_Error from copy_dir() and _copy_dir() if the directory listing failed.

Props afragen, dd32.
Fixes #52342.

#14 @pbiron
4 months ago

  • Description modified (diff)
Note: See TracTickets for help on using tickets.