Save filesize with attachment metadata on upload #493

goldenapples · 2021-02-18T22:18:48Z

Sets the "filesize" property in the attachment metadata when initially uploading images, so that it doesn't have to be queried for in the ajax response on wp_prepare_attachment_for_js().

Fixes #492.

Sets the "filesize" property in the attachment metadata when initially uploading images, so that it doesn't have to be queried for in wp_prepare_attachment_for_js() ajax response.

joehoyle · 2021-02-19T08:18:37Z

Nice, I had assumed core already did this -- do you know why it doesn't?

joehoyle · 2021-02-19T08:45:44Z

inc/class-plugin.php

+	 * @param array $metadata Attachment metadata.
+	 * @return array Attachment metadata array, with "filesize" value added.
+	 */
+	function set_filesize_in_attachment_meta( $metadata ) {


Suggested change

function set_filesize_in_attachment_meta( $metadata ) {

function set_filesize_in_attachment_meta( array $metadata ) : array {

This project is using Psalm too, so once the unit tests are fixed, I'd imagine we'll need to add the array shape decelerating like elsewhere in the file.

joehoyle · 2021-02-19T09:25:53Z

The unit tests are failing here, because metadata['file'], surprisingly is not always set. It seems that's only for images, videos for example never get that set (https://core.trac.wordpress.org/browser/tags/5.6.1/src/wp-admin/includes/media.php#L3532). It's possible to get the file instead with get_attached_file() rather than reading from the metadata for that value. TIL

goldenapples · 2021-02-19T14:37:18Z

I had assumed core already did this -- do you know why it doesn't?

I don't think there was a reason it wasn't added. The logic to check for "filesize" in attachment meta was added in 4.4 (ticket) to support external image libraries and I guess the assumption was that those plugins had to implement attachment metadata themselves. Looks like there's a ticket open to set filesize in this metadata array in core - https://core.trac.wordpress.org/ticket/49412 - and if that gets merged this workaround can be removed.

The attachment metadata array doesn't always have a "file" attribute. This uses the core API for getting the attached file rather than building it from the file path and the upload dir.

This annotation is too damn long. Cutting out the keys which aren't relevant so that we don't have to define types for all of them.

joehoyle · 2021-02-19T17:33:43Z

inc/class-plugin.php

+	function set_filesize_in_attachment_meta( array $metadata, int $attachment_id ) : array {
+		$file = get_attached_file( $attachment_id );
+
+		if ( file_exists( $file ) ) {


I think this should return early if filesize is already set too, that will mean it's future compatible when images add it.

Yeah, I was thinking that too. Adding it now.

Fixed in 4c50bf1

joehoyle · 2021-02-19T17:34:26Z

inc/class-plugin.php

+	 *
+	 * @param array{file?: string} $metadata      Attachment metadata.
+	 * @param int                  $attachment_id Attachment ID.
+	 * @return array{file?: string, filesize: int} Attachment metadata array, with "filesize" value added.


This should also be filesize? as it's possible the array won't contain it (if the file doesn't exist). I'd have thought Psalm would have caught that 🤔

Makes sense, but that didn't seem to fix the build either 🤔

goldenapples · 2021-02-19T17:35:04Z

@joehoyle Could you help me understand why the build isn't passing here?

The error is

Running Psalm...

Problem parsing /code/psalm.xml:
  Error on line 2:
    Element '{https://getpsalm.org/schema/config}psalm', attribute 'requireVoidReturnType': The attribute 'requireVoidReturnType' is not allowed.

but I'm not familiar with what that means, and I think I'm following the style from the documentation and elsewhere in the file.

Forward compatability check in case WP included this property in the future, see https://core.trac.wordpress.org/ticket/49412.

joehoyle · 2021-02-19T18:38:46Z

Ahh doh it seemed I inadvertently switched to a different version of Psalm ( : void on functions that don't return anything is now required). Will fix up.

joehoyle · 2021-02-19T19:42:06Z

inc/class-plugin.php

+	 */
+	function set_filesize_in_attachment_meta( array $metadata, int $attachment_id ) : array {
+		$file = get_attached_file( $attachment_id );
+		if ( ! $file ) {


@goldenapples get_attached_file can return false, so I added the check here -- Psalm was complaining about that. Yey for type safety!

hm-backport · 2021-03-09T11:34:07Z

The backport to v2-branch failed:

Commits ["e1222c65f467fc88189aa4a79283f47fe75cb7f3","fe01c24c1c5ff6f2f48dc1574bf28b7e67baa303","ce82bc4e1667f9dd06237c2343683237955bc170","4ff03c148127ea23d08dfe70c39973f9b0752935","018a780c4af7c4af14dcc4c0bfccb8e15ce35199","7564fbc3afb30dfe7861efaf4fe581675c812655","4c50bf1117ef12dbefb9b504d7279820b3ced085","069cab852955a1e09a57bc65d31a48cada650d6b","ec01ea6a517f4aaa2fdea501a843494728a9b803","20d54076437f1cb7724acd60caae323b825afd41"] could not be cherry-picked on top of v2-branch

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub.
git fetch
# Create new working tree.
git worktree add .worktrees/backport v2-branch
# Navigate to the new directory.
cd .worktrees/backport
# Cherry-pick all the commits of this pull request and resolve the likely conflicts.
git cherry-pick e1222c65f467fc88189aa4a79283f47fe75cb7f3 fe01c24c1c5ff6f2f48dc1574bf28b7e67baa303 ce82bc4e1667f9dd06237c2343683237955bc170 4ff03c148127ea23d08dfe70c39973f9b0752935 018a780c4af7c4af14dcc4c0bfccb8e15ce35199 7564fbc3afb30dfe7861efaf4fe581675c812655 4c50bf1117ef12dbefb9b504d7279820b3ced085 069cab852955a1e09a57bc65d31a48cada650d6b ec01ea6a517f4aaa2fdea501a843494728a9b803 20d54076437f1cb7724acd60caae323b825afd41
# Create a new branch with these backported commits.
git checkout -b backport-493-to-v2-branch
# Push it to GitHub.
git push --set-upstream origin backport-493-to-v2-branch
# Go back to the original working tree.
cd ../..
# Delete the working tree.
git worktree remove .worktrees/backport

Then, create a pull request where the base branch is v2-branch and the compare/head branch is backport-493-to-v2-branch.

Backport #493 to v2

Save filesize with attachment metadata on upload

e1222c6

Sets the "filesize" property in the attachment metadata when initially uploading images, so that it doesn't have to be queried for in wp_prepare_attachment_for_js() ajax response.

hm-linter bot approved these changes Feb 18, 2021

View reviewed changes

joehoyle reviewed Feb 19, 2021

View reviewed changes

goldenapples added 5 commits February 19, 2021 11:46

Add type hinting to function signature

fe01c24

Get attached file with get_attached_file

ce82bc4

The attachment metadata array doesn't always have a "file" attribute. This uses the core API for getting the attached file rather than building it from the file path and the upload dir.

Add array shape description for psalm

4ff03c1

Remove bonus space

018a780

Only annotate properties actually used

7564fbc

This annotation is too damn long. Cutting out the keys which aren't relevant so that we don't have to define types for all of them.

joehoyle requested changes Feb 19, 2021

View reviewed changes

goldenapples added 2 commits February 19, 2021 12:37

Don't restat file if filesize is already in metadata

4c50bf1

Forward compatability check in case WP included this property in the future, see https://core.trac.wordpress.org/ticket/49412.

Mark filesize property as optional.

069cab8

joehoyle added 2 commits February 19, 2021 20:39

Merge branch 'master' into 492-save-filesize-on-upload

ec01ea6

Check possible false value from get_attached_file()

20d5407

joehoyle reviewed Feb 19, 2021

View reviewed changes

joehoyle approved these changes Feb 19, 2021

View reviewed changes

joehoyle merged commit a840701 into master Feb 19, 2021

joehoyle deleted the 492-save-filesize-on-upload branch February 19, 2021 19:49

tareiking mentioned this pull request Mar 3, 2021

Update S3 uploads dependency humanmade/altis-cloud#333

Closed

roborourke added the backport v2-branch label Mar 9, 2021

goldenapples mentioned this pull request Mar 11, 2021

Backport #493 to v2 #497

Merged

joehoyle added a commit that referenced this pull request Mar 16, 2021

Merge pull request #497 from humanmade/backport-493-to-v2

2064c46

Backport #493 to v2

dependabot bot mentioned this pull request Mar 17, 2021

Bump humanmade/s3-uploads from 2.2.2 to 2.2.3 in /wordpress/base_plugins roadmanict/bazel-monorepo#285

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save filesize with attachment metadata on upload #493

Save filesize with attachment metadata on upload #493

goldenapples commented Feb 18, 2021

joehoyle commented Feb 19, 2021

joehoyle Feb 19, 2021

joehoyle Feb 19, 2021

joehoyle commented Feb 19, 2021

goldenapples commented Feb 19, 2021

joehoyle Feb 19, 2021

goldenapples Feb 19, 2021

goldenapples Feb 19, 2021

joehoyle Feb 19, 2021

goldenapples Feb 19, 2021

goldenapples commented Feb 19, 2021

joehoyle commented Feb 19, 2021

joehoyle Feb 19, 2021

hm-backport bot commented Mar 9, 2021

	function set_filesize_in_attachment_meta( $metadata ) {
	function set_filesize_in_attachment_meta( array $metadata ) : array {

Save filesize with attachment metadata on upload #493

Save filesize with attachment metadata on upload #493

Conversation

goldenapples commented Feb 18, 2021

joehoyle commented Feb 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joehoyle commented Feb 19, 2021

goldenapples commented Feb 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goldenapples commented Feb 19, 2021

joehoyle commented Feb 19, 2021

Choose a reason for hiding this comment

hm-backport bot commented Mar 9, 2021