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

[johnbillion]

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.

[spacedmonkey]

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

[azouamauriac]

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

[azouamauriac]

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

[ironprogrammer]

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.

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

[mukesh27]

Hi there!

PR that changes @filesize( $file ) code in file ​https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-admin/includes/class-wp-filesystem-direct.php#L490

[costdev]

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.

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

[audrasjb]

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

[audrasjb]

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

[mukesh27]

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

[audrasjb]

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

[peterwilsoncc]

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.

[costdev]

@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().

[spacedmonkey]

Good feedback @costdev.

I am happy to commit 55678.diff.

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

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

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

[peterwilsoncc]

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.

https://core.trac.wordpress.org/changeset/53372

[peterwilsoncc]

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 change will add two filters to the class, which it and its parent previously forwent entirely (aside from deprecation handling). Please reconsider.

Please reconsider.

Why? What is the issue with adding filters here?

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

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

[SergeyBiryukov]

[53372] looks good to backport.

[SergeyBiryukov]

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.