REST API: Normalize non-integer attachment filesize metadata#12611
REST API: Normalize non-integer attachment filesize metadata#12611apermo wants to merge 17 commits into
Conversation
Attachment metadata is untyped, so `filesize` can be stored as a string (for example, plugins that populate it from a remote storage API response such as the Google Drive `fileSize` field, which the API returns as a string). Returning it verbatim violated the `?int` return type of `WP_REST_Attachments_Controller::get_attachment_filesize()` and caused a fatal TypeError for non-numeric values. Only return the stored value when numeric, casting to int; otherwise fall through to recompute the size from the file via wp_filesize(). Fixes https://core.trac.wordpress.org/ticket/65670 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
@westonruter would you be able to review this? It fixes a fatal |
|
@westonruter Would you be so kind to have a look at it? Thanks :) |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
Shorten the inline comment in get_attachment_filesize() to the essential rationale, per PR review feedback. See https://core.trac.wordpress.org/ticket/65670 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…HPStan issues in tests
There was a problem hiding this comment.
Pull request overview
This PR fixes a REST API fatal error in WP_REST_Attachments_Controller::get_attachment_filesize() by ensuring attachment filesize metadata (which is untyped postmeta) is normalized to an integer (or recomputed) before being returned under a strict ?int return type.
Changes:
- Normalize numeric
filesizemetadata to anint, and fall back to recomputing the file size when metadata is not usable. - Add REST API regression tests covering numeric-string and non-numeric
filesizemetadata cases. - Add an extra sanity assertion in an existing attachment REST test to ensure the factory returns an integer attachment ID.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/phpunit/tests/rest-api/rest-attachments-controller.php | Adds regression tests validating filesize normalization and non-numeric recovery behavior. |
| src/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php | Hardens get_attachment_filesize() by validating/casting stored metadata before returning it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Refine the filesize metadata guard per PR review: accept only a positive integer (an int or a digit-only string), rather than any numeric value. This avoids silently truncating float strings such as '123.4' via the int cast, and rejects zero and negative values, falling through to recompute the size from the file in those cases. Also document the method's positive-integer return with a @phpstan-return non-negative-int|null annotation. See https://core.trac.wordpress.org/ticket/65670 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Attachment metadata is untyped, so `filesize` may be a float, bool or other non-string scalar. Passing such a value to ctype_digit() is fragile (deprecated in PHP 8.1+), so guard the call with is_string(); non-string scalars now fall through to recompute the size from the file. Extend the invalid-value data provider with float and boolean cases. See https://core.trac.wordpress.org/ticket/65670 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
I fixed the typing of this function as otherwise get_attachment_filesize() would not be guaranteed to return a non-negative-int. This would warrant additional tests to ensure negative values get increased to zero.
Could there be any reason any plugin would filter pre_wp_filesize or wp_filesize to be negative?
There was a problem hiding this comment.
Besides the obvious "Developer did a bad job, or had some quirky idea"?
I don't think so, but that alone is already bad enough.
There was a problem hiding this comment.
No technically legitimate reason, but definitely a reason.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
tests/phpunit/tests/rest-api/rest-attachments-controller.php:1026
wp_update_attachment_metadata()replaces the entire attachment metadata array. In this test, passingarray( 'filesize' => ... )wipes any existing metadata keys (e.g. width/height/sizes), which can make the test less representative and may hide issues in response preparation. Consider merging into the existing metadata (or an empty array if none exists) and updating only thefilesizekey.
wp_update_attachment_metadata( $attachment_id, array( 'filesize' => '123456' ) );
tests/phpunit/tests/rest-api/rest-attachments-controller.php:1057
wp_update_attachment_metadata()overwrites the full metadata array, so setting onlyfilesizehere discards any existing keys and can make the test less realistic. Prefer reading the current metadata (or using an empty array) and updating only thefilesizeentry before writing it back.
wp_update_attachment_metadata( $attachment_id, array( 'filesize' => $filesize ) );
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| ( | ||
| is_int( $meta['filesize'] ) || | ||
| ( is_string( $meta['filesize'] ) && ctype_digit( $meta['filesize'] ) ) | ||
| ) && |
There was a problem hiding this comment.
In reality, I think going through all this is overkill.
| ( | |
| is_int( $meta['filesize'] ) || | |
| ( is_string( $meta['filesize'] ) && ctype_digit( $meta['filesize'] ) ) | |
| ) && | |
| is_numeric( $meta['filesize'] ) && |
Trac ticket
https://core.trac.wordpress.org/ticket/65670
Problem
WP_REST_Attachments_Controller::get_attachment_filesize()(new in 7.0.0) returns thefilesizevalue from attachment metadata verbatim against a strict?intreturn type. Attachment metadata is untyped postmeta, so a non-numeric string value throws a fatalTypeErrorwhen the media endpoint is requested:A numeric string only survives because the file has no
strict_typesdeclaration (coercion). A non-numeric one fatals.Plugins populate
filesizefrom remote storage API responses — e.g. the Google DrivefileSizefield (returned as a string) via WP Media Folder, and the equivalent Dropbox/OneDrive addons — so this occurs on real sites.Fix
Only return the stored value when it is numeric, cast to
int; otherwise fall through to recompute the size from the file viawp_filesize()(which always casts to int), returningnullif unavailable. This matches the'type' => 'integer'REST schema.Testing
Two regression tests added to
tests/phpunit/tests/rest-api/rest-attachments-controller.php:filesizemeta → normalized tointin the response;filesizemeta → no fatal, falls back to the real file size.The non-numeric test reproduces the reported
TypeErroragainst unpatchedtrunk(red), and passes with the fix (green).Disclosure
Claude (Anthropic) assisted in diagnosing the root cause and drafting the patch and tests. I reviewed the change, verified the red/green test behavior locally, and take responsibility for it.