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 | Owned by: | 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)
Change History (31)
#3
@
3 years ago
oooops! seems like the browser removes the space itself... So 55678.2.diff and 55678-1.diff are same.
#4
@
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
- From a Terminal window, navigate to the WordPress root directory and run:
wp shell
- At the
wp>
prompt, enter:$size = wp_filesize( 'file-not-exist.php' );
(assuming that this path is not an actual file). - ✅ Observe that the
wp-cli
shell returnsint(0)
and that noE_WARNING
message has been generated in the debug log. This confirms use of error suppression using@filesize()
from #49412. - At the
wp>
prompt runexit
to return to the command line shell.
Steps to Test Patch
- 🛠 Apply patch.
- Repeat Steps 1-2 above.
- ✅ Observe that the
wp-cli
shell returnsint(0)
and that noE_WARNING
message has been generated in the debug log. This confirms mitigation of afilesize()
error message without using the error suppression operator, but by utilizing anis_readable()
check in the new patch. - 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 PR #2677 on WordPress/wordpress-develop by mukeshpanchal27.
3 years ago
#5
Trac ticket: https://core.trac.wordpress.org/ticket/55678
#6
@
3 years ago
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
This ticket was mentioned in Slack in #core by costdev. View the logs.
3 years ago
#8
follow-up:
↓ 12
@
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.
This ticket was mentioned in PR #2682 on WordPress/wordpress-develop by audrasjb.
3 years ago
#9
Trac ticket: https://core.trac.wordpress.org/ticket/55678
#10
@
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
@
3 years ago
- Keywords dev-reviewed added
Tests are passing and the logic looks good to me. Marking as dev-reviewed
.
#13
@
3 years ago
Just noting that the current PR still needs another core committer sign off before commit :)
#14
@
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
@
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()
.
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.
peterwilsoncc commented on PR #2694:
3 years ago
#20
#21
@
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
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
3 years ago
#26
I understand. This pull is a duplicate of https://github.com/WordPress/wordpress-develop/pull/2402.
I would recommend using
is_readable
instead of file exists. File exist is not the only thing that is important here.