Skip to content

REST API: Normalize non-integer attachment filesize metadata#12611

Open
apermo wants to merge 17 commits into
WordPress:trunkfrom
apermo:trac-65670-attachment-filesize-typeerror
Open

REST API: Normalize non-integer attachment filesize metadata#12611
apermo wants to merge 17 commits into
WordPress:trunkfrom
apermo:trac-65670-attachment-filesize-typeerror

Conversation

@apermo

@apermo apermo commented Jul 20, 2026

Copy link
Copy Markdown

Trac ticket

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

Problem

WP_REST_Attachments_Controller::get_attachment_filesize() (new in 7.0.0) returns the filesize value from attachment metadata verbatim against a strict ?int return type. Attachment metadata is untyped postmeta, so a non-numeric string value throws a fatal TypeError when the media endpoint is requested:

TypeError: WP_REST_Attachments_Controller::get_attachment_filesize():
Return value must be of type ?int, string returned

A numeric string only survives because the file has no strict_types declaration (coercion). A non-numeric one fatals.

Plugins populate filesize from remote storage API responses — e.g. the Google Drive fileSize field (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 via wp_filesize() (which always casts to int), returning null if unavailable. This matches the 'type' => 'integer' REST schema.

Testing

Two regression tests added to tests/phpunit/tests/rest-api/rest-attachments-controller.php:

  • numeric-string filesize meta → normalized to int in the response;
  • non-numeric filesize meta → no fatal, falls back to the real file size.

The non-numeric test reproduces the reported TypeError against unpatched trunk (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.

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>
@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props apermo, westonruter, xate, mukesh27.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@apermo

apermo commented Jul 20, 2026

Copy link
Copy Markdown
Author

@westonruter would you be able to review this? It fixes a fatal TypeError in get_attachment_filesize() (new in 7.0.0) when filesize attachment metadata is stored as a non-numeric string — which happens on sites using plugins that populate it from remote storage APIs (e.g. the Google Drive fileSize string via WP Media Folder). Trac: https://core.trac.wordpress.org/ticket/65670

@apermo

apermo commented Jul 20, 2026

Copy link
Copy Markdown
Author

@westonruter Would you be so kind to have a look at it? Thanks :)

@xateman xateman left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

@github-actions

Copy link
Copy Markdown

Test using WordPress Playground

The 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

  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

apermo and others added 2 commits July 20, 2026 13:41
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>
Copilot AI review requested due to automatic review settings July 21, 2026 02:58

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 filesize metadata to an int, and fall back to recomputing the file size when metadata is not usable.
  • Add REST API regression tests covering numeric-string and non-numeric filesize metadata 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.

Comment thread src/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php Outdated
Comment thread tests/phpunit/tests/rest-api/rest-attachments-controller.php
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>
Copilot AI review requested due to automatic review settings July 21, 2026 06:32

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread src/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php Outdated
Comment thread tests/phpunit/tests/rest-api/rest-attachments-controller.php
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>
Copilot AI review requested due to automatic review settings July 21, 2026 06:39

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings July 21, 2026 07:23

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No technically legitimate reason, but definitely a reason.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

Comment thread src/wp-includes/functions.php
Comment thread tests/phpunit/tests/rest-api/rest-attachments-controller.php Outdated
Comment thread tests/phpunit/tests/rest-api/rest-attachments-controller.php Outdated
Comment thread tests/phpunit/tests/rest-api/rest-attachments-controller.php Outdated
Copilot AI review requested due to automatic review settings July 21, 2026 19:44

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, passing array( '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 the filesize key.
		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 only filesize here discards any existing keys and can make the test less realistic. Prefer reading the current metadata (or using an empty array) and updating only the filesize entry before writing it back.
		wp_update_attachment_metadata( $attachment_id, array( 'filesize' => $filesize ) );

Comment thread tests/phpunit/tests/functions/wpFilesize.php Outdated
Comment thread tests/phpunit/tests/functions/wpFilesize.php Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 21, 2026 19:50
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 21, 2026 19:57
Comment on lines +2374 to +2377
(
is_int( $meta['filesize'] ) ||
( is_string( $meta['filesize'] ) && ctype_digit( $meta['filesize'] ) )
) &&

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In reality, I think going through all this is overkill.

Suggested change
(
is_int( $meta['filesize'] ) ||
( is_string( $meta['filesize'] ) && ctype_digit( $meta['filesize'] ) )
) &&
is_numeric( $meta['filesize'] ) &&

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants