Image Editor: Stabilize endpoint #23536
Conversation
TimothyBJacobs
added
REST API Interaction
TimothyBJacobs
force-pushed
the
change/image-editor-route
branch
from
5289888
to
5519045
Compare
Co-authored-by: Andrew Ozz <743931+azaozz@users.noreply.github.com>
Made some of those changes. Attachments aren't a hierarchical post type, so it wouldn't have a parent. I think I'm going to save the |
|
The file header needs updating too (copy/paste error?) https://github.com/WordPress/gutenberg/pull/23536/files#diff-291983831703ba4378ca5e0aafccb6f8R3-R4 |
|
Fixed the file header. |
No I don't. We really don't have private APIs in PHP. There are a couple of tiny functions which are in no way comparable to an entire REST API endpoint. The closest candidate is the list table API, and I'll mention again, we've maintained BC for it. Again, I'm not strongly against the idea, but IMO it needs wider discussion. |
|
Agreed with @TimothyBJacobs — this is a decision that needs to come from a wider discussion with more core contributors and committers. It's not to say it can't or shouldn't happen, just that it's something that absolutely merits a more formal understanding of what the implications are and how to manage the risk appropriately. |
|
I agree with @TimothyBJacobs . We discussed this matter in the #core-restapi slack channel here. As a rule, once something goes into core, it basically never goes out. A feature like image editing, WILL be useful by plugin developers, even if we nicely ask them not to. If a feature isn't ready for core, it should go in. It can be developed in a gutenberg or a feature plugin. My menus endpoint has been in development for a year. As much as I want it in, until it ready for core and completely stable, it doesn't go in :( |
|
And to the point of unstable JS functionality, the way I rationalize it is that that unstable functionality is that it comes via an external package that gets bundled with core — it just happens to be that those external packages are also developed by the WordPress team. It's harder to make that logic leap with code that's going to be directly maintained in core/Trac. There have already been people asking about using these endpoints outside of Gutenberg. Without a formal discussion and well-communicated understanding of how core intends to treat these endpoints it's very likely they'll be used and abused the same way that other "private" APIs (like the List Table API) have been. |
|
I understand your point of view. Third-party users want these unstable/experimental APIs. That said,it's as important to aknowledge these points:
|
|
@youknowriad I think the point we're trying to raise is that historically WordPress core has not had a formal policy around experimental features. Given WordPress' historical commitment to backward compatibility in core code, it seems reasonable to discuss in a core meeting (perhaps even this week) to understand community concerns and what the team here is looking to accomplish, and determine a policy. There are likely other concerns here that we're not thinking of: what effect this might have on auto-updates inadvertently breaking things? What if other parts of core adopt the API (since technically they're first party consumers and can adopt it) but have their implementations broken? Perhaps there are other teams who would like to create experimental or unstable endpoints who have specific concerns that need to be accommodated as well. |
Allow me to disagree with this. Gutenberg has set a formal policy for this for some time now, and Gutenberg is WordPress Core as far as I know. We also didn't come up with this in Gutenberg, we just followed what was already done on PHP, we just put it in a more formal way to help developers know how to mark their API as experimental or unstable. That said, I understand the point about making this more visible and raising more awareness. That's definitely needed. |
|
@youknowriad I don't think it's ever been communicated that this policy applies to all core code. If that's the case I think it needs to be clearly communicated to team reps and to core committers, as this discussion pretty clearly shows at least some portion of those folks were not aware of this. |
|
It should also probably be moved out of the block editor documentation and into the Core handbook. |
|
I think that it is not for the few people on this thread to decide this. This is a fundamental change on how WordPress and 35%+ of the internet is developed. If this is changing it is need to written up a proposal and get community feedback. After waiting a while to get feedback, only then can we really have this discussion. |
|
I think there is a difference between small internal/narrowly-focused functions flagged with an To my knowledge, we haven't shipped a full PHP class as private since If we don't feel confident that something is "ready" for core merge, then it should live in the plugin, where it can stay experimental until it's stable enough for a merge. In this context, "ready" means that we're willing to take on the burden of not breaking things just because we want it to work differently in the future. That to me is the point of feature plugins. The plugin is the experimental label. I'm not saying experimental APIs are bad. Just that to me it doesn't align with traditional core project philosophies. I also understand that this has been done with JS in Gutenberg for a while, but I have always disagreed with those things coming over into core still labeled as experimental. I think Gutenberg has initiated a fundamental shift in how WP thinks about backward compatibility, but it's mostly been confined to the work inside the Gutenberg repo. It spills over into core through the packages being consumed, but in general it's been isolated to that. If we're going to move toward a looser philosophy of shipping bigger—and what will be likely popular—APIs that we plan to break in the future, then I think it would be good to see some guidance from the project lead about where the project's responsibility toward backward compatibility ends. |
|
I think there's a misunderstanding here. The feature is not the "endpoint", it's the image editing and it's finished and stable. But the feature can't be shipped without leaking an API that we don't want to leak specifically to ensure backward compatibility. |
The feature is the REST API + the UI to use that API. The UI is meaningless without the REST API to do the work on changing and saving the image. Anything that goes into core needs unit tests and for the community to test the functionality. What is the point of merging functionality we are going to rewrite later? It is a waste time to test this to the scale that 35%+ of the internet deserves, to then rewrite it months later. We are being kinder to the community, by shipping something that can remain in core, has solid test coverage and will not change. |
|
These two statements seem to contradict since the image editing is dependent on the endpoint and might change as soon as 5.6:
Regardless of what core feature is using the endpoint, if we have serious concerns that we are going to need to break it, I think it's a sign it's not ready to ship in core. Plugins will use it whether we want them to or not. You can't prevent it from leaking once it's in core, but the Gutenberg plugin drastically limits that exposure. This is something very different in WordPress vs more contained/controlled software. Once something is in the wild, we can't expect to control it with a label. The more experimental APIs we ship then break, the more dangerous core updates become for people because we can't count on plugins to not leverage them. The average user isn't going to know or care that a plugin was using an experimental API, just that they updated and their site broke. I don't think we can continue our goal of moving toward widespread trust in automatic updates while simultaneously shipping unstable APIs. So I think it's a question of risk vs project goals and core philosophies. |
We have concerns about breaking the endpoint not the feature (the image resizing). I don't see any contradiction there, it's just implementation details that can change. |
I also don't feel very good when reading this kind of sentence. Gutenberg philosophies are Core philosophies whether we like them or not. It's been 4 major releases that it's on Core now. |
|
Another thing, the decision to make this endpoint stable comes from the REST API team, so the Rest API team considers the endpoint stable. I personally don't know (or even don't care much about this), what I care about is that the "editing image" feature is stable and should be released and that private APIs (no matter what API is this) are a good thing in general. |
Gutenberg has always lived at an arms' length from core, with its own issue management, project management system, a separate contributor handbook, and major development decisions made in isolation and often in private. It's hard for me to make the leap to "Gutenberg changes are pulled into WordPress core, therefore WordPress core must adopt Gutenberg philosophies". Now I want to be explicitly clear — I'm not arguing in favor or against unstable APIs. Yes, I have my own opinions, but the only point I really care about is communication. I know WordPress isn't a democracy, and I do grant that there is a lot that core can learn from Gutenberg, but it just feels like these things should be communicated in a formal way, either via a core dev chat or a Make post or in the core handbook so all contributors and users have a crystal clear understanding of how the project thinks. I don't think that's too much to ask. |
Reiterating my comment from above.
If it is shipped as
For the editing image feature to be considered stable, the backend code also must be considered stable. They can't be separated like that.
Gutenberg JavaScript does, but Core PHP does not.
WordPress has done quite well with a high commitment to backwards compatibility. What got us here won't get us there etc..., but that also doesn't mean we should just break backwards compatibility when unnecessary. A common argument I've seen for why Gutenberg needs a more lenient BC policy is because of bundle size. This clearly isn't a consideration for PHP APIs.
If an API is a candidate for being broken, it shouldn't be shipped in Core to begin with.
As @chrisvanpatten points out, it pretty clearly applies to the Gutenberg ( JS ) code base. And while the policy is formally written, I have had a hard time finding where it was formally discussed. It looks like it was made in an impromptu decision in #core-editor. And was documented without much discussion. The clarification of the docs also did not garner a lot of feedback. I think part of the reason for that is because it was about facilitating the block editor's goals by the block editor team. With the expectation that it wouldn't apply to Core's PHP BC philosophy. The difference in feedback is really noticeable compared to things like https://make.wordpress.org/core/2019/12/09/proposed-javascript-coding-standards-revisions-for-prettier-compatibility/ If you look at the slack backscroll when we discussed this, I think it's clear I'm not opposed to this idea and I'll also echo Chris' statement about communication. |
It also looks like the original intent was to remove the flag when merging to core.
|
|
Here is the question, how do we plan to break this API? Different params or names? We could refactor the internal logic and still keep BC. If it is adding more options then, this again can be done without breaking anything. There are apis in core we have had to do some weird stuff to maintain BC, we could do this here. |
Yeah because this is an RPC endpoint, with our current endpoint design I'm not too worried about maintaining BC. |
|
Wow, lots of (partially related) discussion here :)
My take would be to make it "extremely clear" that it is "doing it wrong" to use. Perhaps Currently there are 340 private functions in core, out of 3447. 10% is a significant number. It's true that WordPress is trying to keep them working in a backwards compatible way, and that unneeded private functions are deprecated, not removed. The same approach can probably be applied to the API: deprecated functions and params typically throw "doing it wrong" notices. |
Are you up for doing it and maintaining it? :) |
|
In the interest of making a decision here, unless a lead says no :), my plan at this point is to merge this version of the endpoint as a proper
Yes, I'm not very worried about maintaining BC with our current design. We can seamlessly transform the old request format to the new request format. And we aren't passing the request object to any hooks in the endpoint. |
Sounds good. Shall we add the end point to trunk now? (The class is conditionally loaded in Gutenberg). Then this will be using trunk's end point. Not a big difference but when committed to trunk it may bring some more feedback, especially about the new filter on |
|
Yep! Working on that patch today. |
Description
wp/v2and the endpoint to/media/<id>/edit.How has this been tested?
Manually tested editing an image.
Types of changes
Breaking change.
Checklist: