Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#52192 closed task (blessed) (fixed)

REST API: Add batch image editing

Reported by: ajlende's profile ajlende Owned by: antpb's profile antpb
Milestone: 5.7 Priority: normal
Severity: normal Version: 5.7
Component: REST API Keywords: has-patch needs-testing
Focuses: Cc:

Description

Moving over from WordPress/gutenberg#23369 since changes are now only in core.

This proposes a new API for image editing that should be more flexible and extensible than the existing API. The new API can take an array of modifiers that will be applied in the order they appear.

Change History (24)

This ticket was mentioned in PR #845 on WordPress/wordpress-develop by ajlende.


3 years ago
#1

  • Keywords has-patch added

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

See: https://github.com/WordPress/gutenberg/pull/23369

This proposes a new API for image editing that should be more flexible and extensible than the existing API. The new API takes an array of modifiers that will be applied in the order they appear.

An example request body looks like this.

{{{json
{

"modifiers": [

{

"type": "crop",
"args": {

"left": 12.875,
"top": 0.125,
"width": 50,
"height": 50

}

},
{

"type": "rotate",
"args": {

"angle": 90

}

}

]

}
}}}

Crop values are as a percentage of the original image and rotation angle is in degrees clockwise.

The crop values need to be percentages because the image loaded on the frontend may not be a scaled version of the original image, so naturalWidth and naturalHeight cannot be trusted as the actual width and height of the original image.

The rotation value is in degrees to avoid rounding errors for the types of rotations that are most common like 90 degrees. Clockwise versus anticlockwise may be up for debate. When using degrees it seems more natural to me to rotate clockwise, but mathematically, when you've converted to radians, a positive value indicates an anticlockwise rotation.

Different front-end implementations may apply edits in a different order, so passing an array will allow them to use their edits as-is without converting to a fixed order.

One example in the wild that actually uses both orders in different contexts is BeFunky. To see the difference, try one of their demo images with a horizon in it so you have a line that you can pay attention to. Then use the 'straighten' option followed by a 'horizontal flip' to see that the angle of the horizon hasn't changed even though you flipped the image. This means that the flip happens before the rotation. If you use the 'rotate' option instead of 'straighten, the rotation happens before the flip.

The old API continues to work, but when supplying the modifiers option, the old crop and rotation options are ignored.

#2 @TimothyBlynJacobs
3 years ago

  • Milestone changed from Awaiting Review to 5.7

Thanks for the patch @ajlende!

#3 @ajlende
3 years ago

Yeah, and sorry for the delay on moving it here. It's been sitting in my queue of things to work on for quite a while and I've only finally gotten around to it again. You always seem to be more on top of things than I am, so thank you!

TimothyBJacobs commented on PR #845:


3 years ago
#4

Let's also move the deprecated modifiers to the end of the args description.

ajlende commented on PR #845:


3 years ago
#5

I know you had mentioned adding a new deprecated option, but I was weary of adding a non-standard key on the JSON Schema, so it seemed like just adding a notice to the description may be sufficient.

I can still go ahead and add that option if you still think it's needed—something like array( 'deprecated' => ( 'Use modifiers instead.' ) ) maybe rather than just a boolean.

And before we go ahead and merge this, I'd like @azaozz's opinion on updating it to use the args key which I had mentioned in the old PR, but hadn't changed until moving it here.

TimothyBJacobs commented on PR #845:


3 years ago
#6

Let's leave a formal deprecated syntax for another ticket.

This ticket was mentioned in Slack in #core-media by ajlende. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-media by hellofromtonya. View the logs.


3 years ago

#9 @hellofromTonya
3 years ago

Status: needs a review from the Media Team. cc @antpb

#10 @Mista-Flo
3 years ago

  • Keywords needs-testing added

Hi there, code looks good.

Can we have some steps to reproduce/test the endpoint change, so that we can validate the new behaviour.

#11 @ajlende
3 years ago

@Mista-Flo There are two things that need testing: the current API and the new API.

This is not a breaking change for the API, so the current/old API should still work. Test inline image editing with the image block in Gutenberg as it is.

Second is that the new API works. I have a Gutenberg branch that uses the new API at https://github.com/WordPress/gutenberg/pull/28530. You can use that branch to edit an image with the inline image editor in the image block.

This ticket was mentioned in Slack in #core-media by mkaz. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-media by hellofromtonya. View the logs.


3 years ago

#14 @antpb
3 years ago

Hi @ajlende ! Thanks for the PR! I was able to test over the weekend and all seems good. There's just one problem that I can see and that's the failing e2e tests around rotating images. Here's a link to the failure: https://github.com/WordPress/gutenberg/pull/28530/checks?check_run_id=1779963046

I'm going to look into it as it seems related to the changes here.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


3 years ago

#16 @antpb
3 years ago

  • Owner set to antpb
  • Resolution set to fixed
  • Status changed from new to closed

In 50124:

REST API, Media: Add batch image editing endpoints.

Introduces new endpoints to allow for batch image editing using the REST API.

The new endpoints can take an array of modifiers that will be applied in the order they appear.

Props ajlende, TimothyBlynJacobs, hellofromTonya, Mista-Flo.
Fixes #52192.

#17 @SergeyBiryukov
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

DEPRECATED: Use modifiers instead

Putting this in translatable descriptions doesn't seem to follow any other patterns in core. Is that necessary? Is there anything wrong with using the older format, apart from the newer one taking precedence?

If we want everyone to stop using the older format, could we call _deprecated_argument() instead when any of these older arguments are used? That would also allow us to specify the version in which they were deprecated.

At the very least, modifiers should be moved out of the translatable strings and replaced with a placeholder, to prevent it from being translated.

Version 2, edited 3 years ago by SergeyBiryukov (previous) (next) (diff)

#18 @ajlende
3 years ago

The Gutenberg failures are expected since they require the core change. I just caught up on the slack linked above, and I second everything @TimothyBlynJacobs said.

I think that failing might be expected since the new endpoint isn’t in Core yet.

The draft PR I had requires the updated endpoint, thus the failures. As detailed below, the GB plugin supports older core versions, so there's still more work to be done to add support when it isn't there.

Core should be first.

This is a backwards-compatible change. The existing Gutenberg code should still work with this change as described in the testing procedure.

The GB PR is going to need adjustment anyways because the plugin supports back to 5.5 IIRC.

The plugin page suggests 5.3. Typically we check for feature support and patch in the required updates if it isn't available. I'll have to include that in the GB PR before it gets merged.

#19 @TimothyBlynJacobs
3 years ago

The plugin page suggests 5.3. Typically we check for feature support and patch in the required updates if it isn't available. I'll have to include that in the GB PR before it gets merged.

The plugins page is outdated: https://github.com/WordPress/gutenberg/issues/28075

@SergeyBiryukov AFAIK, we haven't deprecated any REST API parameters yet, this is the first. At a later point I plan on adding a more formal deprecated syntax that we could apply to the parameter schemas. So at the moment, this is just a soft deprecation.

The problem generally is that we don't have a good way to generate changelogs for the REST API. We could add @since markers, but they aren't picked up by the REST API reference documentation. We also haven't really done this in the past. AFAICT the only usages are in WP_REST_Posts_Controller::get_collection_params(). Ideally, we'd make this available in the schema definition for each parameter.

Putting Deprecated in the parameter description means that it will be picked up by the REST API reference. Though for some reason, the edit endpoint isn't being picked up at all. I'll take a look at that when we regenerate our docs for 5.7.

#20 @desrosj
3 years ago

In 50152:

Coding Standards: Fix several minor coding standards issues.

These are made by running composer format.

Follow up to [50124], [50129], [50143].

See #49961, #52192, #34281.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 years ago

#22 @hellofromTonya
3 years ago

  • Type changed from enhancement to task (blessed)

Reclassifying the possible refinement effort to task as we are past Beta 1 (ie hard stop for all enhancements).

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 years ago

#24 @SergeyBiryukov
3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Re-closing as fixed for 5.7 in [50124]. Created #52629 to address comment:17.

Note: See TracTickets for help on using tickets.