Make WordPress Core

Opened 2 years ago

Last modified 2 years ago

#55688 new defect (bug)

Update size function in WP_Filesystem_Direct

Reported by: mukesh27's profile mukesh27 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 2.5
Component: Filesystem API Keywords: has-patch close dev-feedback
Focuses: Cc:


Related: #55678

Replying to costdev:

PR2677 was discussed in the scrub. The PR patches a different function and this should be handled in a different ticket.

Change History (3)

#2 @SergeyBiryukov
2 years ago

Thanks for the PR!

This change has backward compatibility concerns, as the method would no longer return false on failure, and it would not be possible to tell the difference between a file of 0 bytes or an error reading the file size. That might seem acceptable for media functions, for which wp_filesize() was introduced, but not for the WP_Filesystem_* classes, which are heavily used for updating plugins, themes, and WordPress core itself.

So I'm not sure this should be changed at all, especially in 6.0, which is only two weeks away, and only regressions are addressed at this point.

Frankly, I'm not a fan of the type casting to int in wp_filesize() and would prefer for it to mirror the native PHP function, with additional filters. However, it looks like it was suggested in comment:8:ticket:49412 mostly for ease of use where the difference between a zero size and an error doesn't matter that much, and I can see that point as well.

#3 @costdev
2 years ago

  • Component changed from Media to Filesystem API
  • Keywords close dev-feedback added
  • Milestone changed from 6.0 to Awaiting Review
  • Version changed from trunk to 2.5

As this isn't a regression introduced during the 6.0 cycle, I'm removing this from the milestone.

As Sergey has said, the PR introduces a BC break, as the method can no longer return false, and it's in a mission-critical component.

This change would also make it possible to filter the result via the 'pre_wp_filesize' and 'wp_filesize' filters, so this could end up breaking BC in a much larger way than removing the false return.

I'm adding close as I think this is too risky.

Adding dev-feedback to see what others think about this. If there's a desire to have this method call wp_filesize(), it needs extremely careful consideration. For BC, the checks to cause a false return would need to be run before calling wp_filesize().

Note: See TracTickets for help on using tickets.