Add template revision button #45215
Add template revision button #45215
Conversation
georgeh
force-pushed
the
add/template-revision-button
branch
from
0f824f5
to
be22d83
Compare
|
Size Change: +600 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
georgeh
requested review from
TimothyBJacobs,
spacedmonkey,
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
Thinking more about the REST routes in this, I think I can use the revisions REST controller, but only add the links when there are 2 or more custom versions of a template. As a follow-up it would also be good to let the revision viewer load theme templates from the filesystem as a kind of revision 0 |
|
This button could probably use some design help too:
|
No, it won't work with the routes as they are currently set up. I've learned this twice now so I'm writing it down here for the next time I attempt this.
// The route.
sprintf(
'/%s/(?P<id>%s%s)',
$this->rest_base,
// Matches theme's directory: `/themes/<subdirectory>/<theme>/` or `/themes/<theme>/`.
// Excludes invalid directory name characters: `/:<>*?"|`.
'([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)',
// Matches the template name.
'[\/\w-]+'
)which expands to something like
// $this->rest_base = 'revisions'
'/' . $this->parent_base . '/(?P<parent>[\d]+)/' . $this->rest_base;
// ... and ....
'/' . $this->parent_base . '/(?P<parent>[\d]+)/' . $this->rest_base . '/(?P<id>[\d]+)',which expands to something like Excluding routes that end in |
|
|
||
| if ( rest_is_field_included( 'revision_count', $fields ) ) { | ||
| $data['revision_count'] = (int) $revisions['count']; | ||
| } | ||
|
|
||
| if ( rest_is_field_included( 'latest_revision_id', $fields ) ) { | ||
| $data['latest_revision_id'] = (int) $revisions['latest_id']; | ||
| } |
There was a problem hiding this comment.
Instead of adding the data like this, please add like other endpoints, like this.
There was a problem hiding this comment.
The other endpoints provide links to REST routes that provide revision information, which I inferred as the reason that they are in the _links section in the response. I've noted in the How section of my description and this comment that the REST routes for revisions are not compatible with the routes for templates, because template IDs are non-numeric and use slashes internally.
I chose not to put these in the _links section because of the difficulty getting the Revision REST controller routes to respond. Are you saying you'd still like to have these data in _links without URLs for other REST responses?
There was a problem hiding this comment.
No, I would like to the revisions endpoints working.
There was a problem hiding this comment.
There is no such idea of revisioned templates for file based templates. There will only ever work with ones stored in the database. As we check to see if there are WordPress id based templates. So this is really an issue.
There was a problem hiding this comment.
What do you think about having a different URL structure for templates? Something like /wp/v2/revisions/template[_part]s/123/456? It seems like the revision controller could have a special case for the route for those post types to avoid conflicts.
There was a problem hiding this comment.
There is already an existing endpoint that looks like this.
/wp/v2/templates/(?P<parent>[\\d]+)/revisions":
and
/wp/v2/templates/(?P<parent>[\\d]+)/revisions/(?P<id>[\\d]+)":
There is no need for a different endpoint / structure.
There was a problem hiding this comment.
Those endpoints do exist, however the routes in WP_REST_Templates_Controller also match those URLs and so WP_REST_Revisions_Controller never sees requests for them. You can confirm this by making a request to those endpoints and seeing that the response is from the Templates controller: the error code is rest_template_not_found.
Here is the best solution I've been able to come up with to route /wp/v2/template-parts/18/revisions and /wp/v2/template-parts/123/revisions/456 to WP_REST_Revisions_Controller:
First, change the route for WP_REST_Templates_Controller::get_item to add (?<!revisions) at the end. This prevents the controller from matching requests for /wp/v2/template-parts/123/revisions. This fixes requests for /wp/v2/template-parts/18/revisions.
This regex will still attempt to match /wp/v2/template-parts/123/revisions/456 because that is indistinguishable from /wp/v2/template-parts/themedir/twentytwentythree/footer (we need to allow for theme dirs, and we have to accept a single slash between theme name and part). (?<!revisions) is a negative lookahead, and they must be fixed width so we can't add \d+ in there to match one-or-more digits.
We may be able to update the regex for the template routes to require at least one non-numeric character in theme and template names if we can guarantee no one is going to name a theme 2023 or a template part 1. If not, I put together this pseudocode that feels incredibly hacky, and not a solution I like, but it should allow us to have WP_REST_Templates_Controller intercept requests for /wp/v2/template-parts/123/revisions/456:
function wp_api_reroute_template_revision_requests( $dispatch_result, $request, $route, $handler ) {
// Check if the request is for a template revision.
if ( is_a( $handler['callback'][0], 'WP_REST_Templates_Controller', true ) ) {
$post_type = $handler['callback'][0]->getPostType();
$post_type_object = get_post_type_object( $post_type );
$post_base = ! empty( $post_type_object->rest_base ) ? $post_type_object->rest_base : $post_type_object->name;
// See if we have a request for get_item()
if ( preg_match( '@/' . $post_base . '/(?P<parent>[\d]+)/revisions/(?P<id>[\d]+)$@',
$request->get_route(),
$get_items_matches ) ) {
$revision_controller = new WP_REST_Revisions_Controller( $post_type );
return $revision_controller->get_item( $get_items_matches );
}
}
}
add_filter( 'rest_dispatch_request', 'wp_api_reroute_template_revision_requests', 10, 4 );This feels very brittle and indirect, and not a particularly elegant solution, but it is a solution nonetheless.
@spacedmonkey I would appreciate your thoughts on the proposed solutions (don't link to revisions, change the URL structure for template revisions, intercept requests for revisions in a filter), and if you have any other ideas on ensuring that the Revision controller gets the requests instead of the Template controller. Thanks again for your work on this.
lib/experimental/class-gutenberg-rest-template-revision-count.php
Outdated
Show resolved
Hide resolved
lib/experimental/class-gutenberg-rest-template-revision-count.php
Outdated
Show resolved
Hide resolved
|
Working on the revisions, thanks for the review @spacedmonkey. Also I opened #45290 as a possible follow-up to make template revisions clearer |
|
Does this PR provide an alternative to WordPress/wordpress-develop#3476, or does it depend on it? |
@priethor That was an earlier attempt on adding to the REST API and this is a better place for that change. I'll close that and we can merge the changes to core using the usual Gutenberg merge process. |
I have reported this issue upstream here - https://core.trac.wordpress.org/ticket/56922. It will hopefully be fixed in WordPress 6.2. |
Thanks @spacedmonkey - I'll get this PR ready for when that gets fixed and hopefully we can backport the fix to this as well. |
|
@georgeh I have pushed some changes to this PR, with a fix. Let me know what you think. |
|
@spacedmonkey thanks, that's about what I was coming up with so it lgtm. How do we feel about merging this when the |
We can fix the issue in core or guternberg PR. |
georgeh
force-pushed
the
add/template-revision-button
branch
from
91a1b07
to
0e4eb33
Compare
|
This is looking good. Let's ensure that unit tests / linting are passing. Then we can merge. |
ntsekouras
added
Backport to WP 6.7 Beta/RC
DaisyOlsen
added
[Type] Enhancement
ntsekouras
added
Backport to WP 6.7 Beta/RC
|
This cannot be in 6.2 because once we land in the revision UI the links to take us back don't work and fixing that is not trivial. |
draganescu
removed
the
Backport to WP 6.7 Beta/RC
|
I was looking at backporting #48078 which essentially merges the I then went to the trac ticket for this PR and saw it was blocked by upstream changes, see at https://core.trac.wordpress.org/ticket/57704#comment:4 It couldn't be backported in 6.3 and, as far as my understanding goes, it cannot be backported in 6.3 either. As for next steps, fixing the upstream issue would be ideal, and that's the main next step. I also wonder how we should mark this code as "blocked by upstream". If the upstream issue is not fixed in time for the 6.4 release, I worry that we may unintentionally remove the revisions related code that lives in
How does this sound? |
Moving files to To me that makes sense. Then it can be relocated again to I can remove the corresponding PR mentions from the 6.3 backport issue if you think we're not going to include it in 6.3? To confirm, this PR and #48078 are to be scratched for a future release... |
|
Here's a PR to move the revisions button API stuff back to |
Move from 6.3 to experimental #45215 (comment)
Move from 6.3 to experimental #45215 (comment)
) Move from 6.3 to experimental WordPress#45215 (comment)
What?
Adds a button to templates and template parts to get to the revision compare screen
Why?
Closes #44503
How?
This adds a button to the site editor for Templates and Template Parts that links to the revision viewer for that template.
It adds 2 new fields to the REST response for
/wp/v2/template/{theme}//{part}and/wp/v2/template-part/{theme}//{part}routes: 'revision_count' andlatest_revision_id. The Posts REST controller uses the_linkssection to expose these and links to routes for the Revisions REST controller. I chose to add these to the body because the Revisions REST routes assume numeric IDs with/revisionsat the end, and the existing template routes receive everything in their namespace. Adding/revisionsroutes for templates and template-parts would require complex RegExes that could potentially lead to a DoS attack. However if someone can get the routes on the Revisions REST controller to work with Template IDs, I would be happy to add them.Testing Instructions
Screenshots or screencast
revisions-style-update.mp4