Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port Edit Image endpoint #388

Closed
wants to merge 5 commits into from

Conversation

TimothyBJacobs
Copy link
Member

Porting the edit image endpoint from Gutenberg.

This is a WIP, working on adding tests. One question I had was the descriptions of the request args, I did my best to add them based on my understanding of how the endpoint works, but that might not be accurate.

Cc: @ajlende.

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


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.


// This also confirms the attachment is an image.
$image_file = wp_get_original_image_path( $attachment_id );
$image_meta = wp_get_attachment_metadata( $attachment_id );
Copy link
Contributor

@azaozz azaozz Jul 3, 2020

Choose a reason for hiding this comment

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

There is a possible rare edge case here: the attachment_id comes from the image block (hard-coded in post_content), but if the site was moved, i.e. posts have been exported and imported at another site, the post IDs may change. Seems this will also need to confirm that $image_meta is for the same image. (Not in scope here, just need to remember to add it).

@azaozz
Copy link
Contributor

azaozz commented Jul 3, 2020

Looks good, just one small nitpick :)

Will also need to verify that $image_meta is actually for the same image as it is retrieved by using the attachment ID that is hard-coded in post_content. (Can patch that in trunk. There is another patch that needs the same and will probably have to make a new helper function.)

return array(
'rotation' => array(
'description' => __( 'The amount to rotate the image clockwise in degrees.' ),
'type' => 'integer',
Copy link
Member Author

Choose a reason for hiding this comment

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

@ajlende can this be constrained to a min 0 and max 360?

Copy link

@ajlende ajlende Jul 6, 2020

Choose a reason for hiding this comment

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

For this version, yes, that's fine. In the next, I'd prefer to not have constraints on it. It's easy enough to convert to the correct range, but if you're rotating 270 degrees, for example, it might be more natural to say -90 instead. I'd assume the implementations for rotating an image in WP_Image_Editor take this into account, so converting to a specific range just seems like an extra step.

@TimothyBJacobs
Copy link
Member Author

Should we be checking against upload_is_user_over_quota @azaozz?

@azaozz
Copy link
Contributor

azaozz commented Jul 3, 2020

Should we be checking against upload_is_user_over_quota

Possibly, but seems better to let the user edit the image, and decide if they should delete the parent image? Users can get over the quota even when uploading a large image that creates many sub-sizes...

Would make sense to return a "non-blocking" error that the user in now over the quota after saving the edited image :)

@TimothyBJacobs
Copy link
Member Author

Possibly, but seems better to let the user edit the image, and decide if they should delete the parent image? Users can get over the quota even when uploading a large image that creates many sub-sizes...

Gotcha. I'll leave it alone for now then.

This works for me testing against the latest Gutenberg. It still doesn't work alone on trunk since the JS in trunk is still expecting the old endpoint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants