Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#55678 closed defect (bug) (fixed)

Avoid `filesize()` warnings when storing file size in media metadata

Reported by: johnbillion's profile johnbillion Owned by: audrasjb's profile audrasjb
Milestone: 6.0 Priority: normal
Severity: normal Version: 6.0
Component: Media Keywords: has-patch commit dev-reviewed fixed-major
Focuses: Cc:

Description

As reported by Cybr in 61:ticket:49412:


filesize() generates an E_WARNING in case of an error (source). The @ operator would suppress that. I see that operator is primarily used when handling files in WP; whether or not justified is concerned in #24780, as mentioned in comment:55.

If we agree not using the Error Suppression operator, it'd be better to use
$size = file_exists( $path ) ? (int) filesize( $path ) : 0;
Casting to (int) is still required because PHP returns false on error, for whatever reason that might happen.

Attachments (3)

55678.diff (391 bytes) - added by antpb 3 years ago.
55678-1.diff (391 bytes) - added by antpb 3 years ago.
55678.2.diff (391 bytes) - added by azouamauriac 3 years ago.
add just space after '=' in 55678-2.diff.

Download all attachments as: .zip

Change History (31)

#1 @spacedmonkey
3 years ago

I would recommend using is_readable instead of file exists. File exist is not the only thing that is important here.

@antpb
3 years ago

@antpb
3 years ago

#2 @spacedmonkey
3 years ago

  • Keywords has-patch added; needs-patch removed

@azouamauriac
3 years ago

add just space after '=' in 55678-2.diff.

#3 @azouamauriac
3 years ago

oooops! seems like the browser removes the space itself... So 55678.2.diff and 55678-1.diff are same.

Last edited 3 years ago by azouamauriac (previous) (diff)

#4 @ironprogrammer
3 years ago

Thank you for the patch, @antpb!

Test Report

55678-1.diff (and 55678.2.diff 😂)

To clarify, this test does not confirm the existence of a defect prior to patching, but rather validates consistent logging behavior expected after the patch is applied.

Environment

  • WordPress 6.0-RC1-53341-src
  • macOS 12.3.1 (Monterey)
  • In wp-config.php: define( 'WP_DEBUG_LOG', true );
  • `wp-cli` is installed

Steps to Reproduce

  1. From a Terminal window, navigate to the WordPress root directory and run: wp shell
  2. At the wp> prompt, enter: $size = wp_filesize( 'file-not-exist.php' ); (assuming that this path is not an actual file).
  3. ✅ Observe that the wp-cli shell returns int(0) and that no E_WARNING message has been generated in the debug log. This confirms use of error suppression using @filesize() from #49412.
  4. At the wp> prompt run exit to return to the command line shell.

Steps to Test Patch

  1. 🛠 Apply patch.
  2. Repeat Steps 1-2 above.
  3. ✅ Observe that the wp-cli shell returns int(0) and that no E_WARNING message has been generated in the debug log. This confirms mitigation of a filesize() error message without using the error suppression operator, but by utilizing an is_readable() check in the new patch.
  4. Repeat Step 4 above.

Expected Results

  • The logging behavior before the patch was reproduced/confirmed successfully. ✅
  • The patch retains the expected logging behavior (i.e. no E_WARNING logged). ✅

Additional Notes

  • Unit tests for wp_filesize() were included in #49412.

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


3 years ago

#8 follow-up: @costdev
3 years ago

  • Keywords commit added

This ticket was reviewed in the bug scrub. Adding the commit keyword.

Note to committers: 55678-1.diff is the commit candidate.

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

#10 @audrasjb
3 years ago

  • Owner set to audrasjb
  • Status changed from new to reviewing

PR added to run unit tests against trunk for 55678-1.diff.

#11 @audrasjb
3 years ago

  • Keywords dev-reviewed added

Tests are passing and the logic looks good to me. Marking as dev-reviewed.

#12 in reply to: ↑ 8 @mukesh27
3 years ago

Replying to costdev:

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

Separate ticket #55688 was created for src/wp-admin/includes/class-wp-filesystem-direct.php

#13 @audrasjb
3 years ago

Just noting that the current PR still needs another core committer sign off before commit :)

#14 @peterwilsoncc
3 years ago

  • Keywords dev-feedback added; dev-reviewed removed

Swapping review keywords, dev-reviewed is intended for once a second committer has signed off.

I'm currently discussing the best conditional to use with another contributor to best match how filesize() works without the need for error suppression.

#15 @costdev
3 years ago

@johnbillion @spacedmonkey Using is_readable() will change the behaviour of wp_filesize().

filesize() still returns the filesize for an unreadable file.

So this change will mean that wp_filesize() will have previously returned 1000000000 for a 1GB unreadable file, but will now return 0.

IMO, this change of behaviour, if intentional, should be handled separately from removing error suppression, and the documentation of wp_filesize() should change to reflect the caveat.

If this change of behaviour isn't intentional, then file_exists() is indeed the best conditional to remove the need for error suppression, as this is a failure condition of filesize().

#16 @spacedmonkey
3 years ago

Good feedback @costdev.

I am happy to commit 55678.diff.

peterwilsoncc commented on PR #2682:


3 years ago
#17

Following discussion on the ticket, it was decided to go with file_exists() to better match the behaviour of PHP.

This ticket was mentioned in PR #2694 on WordPress/wordpress-develop by peterwilsoncc.


3 years ago
#18

https://core.trac.wordpress.org/ticket/55678

Just running the tests to see if they pass on all the things.

#19 @peterwilsoncc
3 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 53372:

Media: Remove error suppression in wp_filesize().

Replace error suppressing in wp_filesize() with a file_exists() check before calling the native PHP filesize() function.

Follow up to [52837].

Props Cybr, johnbillion, spacedmonkey, antpb, azouamauriac, ironprogrammer, mukesh27, costdev, audrasjb, dlh.
Fixes #55678.
See #49412.

#21 @peterwilsoncc
3 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening for merging to the 6.0 branch pending sign off from a second committer.

For preference this will be someone other than @antpb as Anthony wrote the initial patch.

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


3 years ago

sybrew commented on PR #2677:


3 years ago
#23

This change will add two filters to the class, which it and its parent previously forwent entirely (aside from deprecation handling). Please reconsider.

spacedmonkey commented on PR #2677:


3 years ago
#24

Please reconsider.

Why? What is the issue with adding filters here?

spacedmonkey commented on PR #2677:


3 years ago
#25

Doesn't this ticket make sense to do as part of - https://core.trac.wordpress.org/ticket/55376

sybrew commented on PR #2677:


3 years ago
#26

I understand. This pull is a duplicate of https://github.com/WordPress/wordpress-develop/pull/2402.

#27 @SergeyBiryukov
3 years ago

  • Keywords dev-reviewed added; dev-feedback removed

[53372] looks good to backport.

#28 @SergeyBiryukov
3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 53380:

Media: Remove error suppression in wp_filesize().

Replace error suppressing in wp_filesize() with a file_exists() check before calling the native PHP filesize() function.

Follow up to [52837].

Props Cybr, johnbillion, spacedmonkey, antpb, azouamauriac, ironprogrammer, mukesh27, costdev, audrasjb, dlh, peterwilsoncc.
Merges [53372] to the 6.0 branch.
Fixes #55678.
See #49412.

Note: See TracTickets for help on using tickets.