Opened 11 months ago
Last modified 2 days ago
#56922 new defect (bug)
Template / Template parts revision / autosave REST API are broken
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | 6.4 | Priority: | normal |
Severity: | normal | Version: | 4.7 |
Component: | REST API | Keywords: | has-patch has-unit-tests needs-testing changes-requested |
Focuses: | rest-api | Cc: |
Description
The REST API endpoints for autosave and revisions the template and template part are broken. The URL structure for template and template part are different from other post controllers.
Standard post controller.
'/' . $this->rest_base . '/(?P<id>[\d]+)',
Template / template parts.
'/%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-]+'
Revision / autosave endpoints expects
'/' . $this->parent_base . '/(?P<parent>[\d]+)/' . $this->rest_base,
Autosave endpoint expects.
When doing a request like this.
wp/v2/template-parts/1480/revisions
Results in
code
:
"rest_template_not_found"
data
:
{status: 404}
message
:
"No templates exist with that id."
As this post type needs a different url structure, a custom revision / autosave will need to be registered.
Change History (23)
This ticket was mentioned in PR #3533 on WordPress/wordpress-develop by @spacedmonkey.
11 months ago
#1
- Keywords has-patch added
#2
@
11 months ago
- Keywords has-patch removed
- Version set to 4.7
I have made a start on a solution that implements custom autosave / revision endpoints.
#3
@
11 months ago
I think that the path wp/v2/template-parts/1480/revisions
is pretty easy to fix, by adding (?<!/revisions)
to the end of the route regex in WP_REST_Templates_Controller
. This is a negative-lookbehind regex that says "the URL can't end in /revisions".
The harder part is avoiding links to individual revisions like wp/v2/template-parts/1480/revisions/4321
. Negative-lookbehinds must be fixed length, we can't do something like (?<!/revisions/\d+)
because \d+
could be any size and the regex engine doesn't want to backtrack an arbitrary number of steps.
One thought I had was for the REST API to be able to use a callable as a route in addition to a regex. This would allow for more complex logic than a regex allows. However, it may make it more difficult to use register_rest_route()
to override an existing route and make it easy to accidentally add big performance problems.
Another option, that probably won't work due to backwards compatibility, would be to use "The Greatest Regex Trick Ever" (https://www.rexegg.com/regex-best-trick.html), which is to force regexes to include at least one capture group (something in parenthesis) and to ignore routes that simply match. This would allow us to have a regex like
\d+/revisions$|\d+/revisions/d+$|/templates/(?P<id>([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)'[\/\w-]+',)
which would match on \d+/revisions$
or \d+/revisions/d+$
but, since they don't have a capture group, the updated router would treat them as non-matches. Existing routes without capture groups (e.g. /templates
) would have to be updated though.
@TimothyBlynJacobs commented on PR #3533:
8 months ago
#5
What would the autosaves / revisions controllers look like when they need to be custom for templates/template parts.
#6
@
8 months ago
Hello! Is this still something that has a chance to land on 6.2? :) I know it's really late.
This ticket was mentioned in Slack in #core-restapi by andraganescu. View the logs.
4 months ago
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
3 months ago
#9
@
7 weeks ago
- Keywords has-patch needs-refresh needs-unit-tests added
- Milestone changed from Future Release to 6.4
Moving this into 6.4 milestone for visibility and to hopefully wrangle some folks to possibility get it moving forward in time to ship in 6.4.
#10
@
3 weeks ago
Just FYI: I am working on a PR for this issue and am also verifying the solution proposed in https://github.com/WordPress/wordpress-develop/pull/3533.
#11
@
3 weeks ago
@antonvlasenko The current PR is just part of the problem. Basically we need the ablity to register custom autosave / revision endpoints for post types. That way we can fix the regex for just those post types. It is not a complex fix, but it might be a lot of code. I just haven't had time to work on it.
This ticket was mentioned in PR #5194 on WordPress/wordpress-develop by @antonvlasenko.
3 weeks ago
#12
- Keywords has-unit-tests added; needs-refresh needs-unit-tests removed
### What and why?
Fixes https://core.trac.wordpress.org/ticket/56922.
This PR addresses route conflicts between WP_REST_Templates_Controller
and both WP_REST_Revisions_Controller
and WP_REST_Autosaves_Controller
.
When making REST requests to the following endpoints:
wp/v2/templates/<parent_post_id>/revisions
wp/v2/templates/<parent_post_id>/autosaves
wp/v2/template-parts/<parent_post_id>/revisions
wp/v2/template-parts/<parent_post_id>/autosaves
the WP_REST_Templates_Controller::get_item()
method is incorrectly matched as the callback method to process these requests. This misidentification arises from the regular expression that defines the problematic route in the WP_REST_Templates_Controller
class, specifically:
/(?P<id>([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)[\/\w%-]+)
.
In actuality, the aforementioned requests should be handled by the WP_REST_Revisions_Controller::get_items()
and WP_REST_Autosaves_Controller::get_items()
callbacks, depending on whether it's a revisions
or autosaves
request.
### How
This PR modifies the order in which the routes for endpoints in WP_REST_Templates_Controller
, WP_REST_Revisions_Controller
, and WP_REST_Autosaves_Controller
are registered.
Routes in WP_REST_Revisions_Controller
and WP_REST_Autosaves_Controller
are now registered prior to those in WP_REST_Templates_Controller
.
This ensures the correct callback is matched, preventing WP_REST_Templates_Controller::get_item()
from being mistakenly matched for the endpoints listed above.
However, this change is limited to the wp_template
and wp_template_parts
post types to mitigate the risk of introducing new bugs.
#13
@
3 weeks ago
Thank you for the information, @spacedmonkey.
I suggest considering the PR I've just submitted - https://github.com/WordPress/wordpress-develop/pull/5194 - as an alternative solution.
I believe it addresses the issue, assuming my understanding of it is accurate.
What and why?
This PR addresses route conflicts between WP_REST_Templates_Controller
and both WP_REST_Revisions_Controller
and WP_REST_Autosaves_Controller
.
When making REST requests to the following endpoints:
wp/v2/templates/<parent_post_id>/revisions
wp/v2/templates/<parent_post_id>/autosaves
wp/v2/template-parts/<parent_post_id>/revisions
wp/v2/template-parts/<parent_post_id>/autosaves
the WP_REST_Templates_Controller::get_item()
method is incorrectly matched as the callback method to handle these requests.
This misidentification arises from the regular expression that defines the problematic route in the WP_REST_Templates_Controller
class, specifically:
/(?P<id>([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)[\/\w%-]+)
.
In actuality, the aforementioned requests should be handled by the WP_REST_Revisions_Controller::get_items()
and WP_REST_Autosaves_Controller::get_items()
callbacks, depending on whether it's a revisions
or autosaves
request.
How
This PR modifies the order in which the routes for endpoints in WP_REST_Templates_Controller
, WP_REST_Revisions_Controller
, and WP_REST_Autosaves_Controller
are registered.
Routes in WP_REST_Revisions_Controller
and WP_REST_Autosaves_Controller
are now registered prior to those in WP_REST_Templates_Controller
.
This ensures the correct callback is matched, preventing WP_REST_Templates_Controller::get_item()
from being mistakenly matched for the endpoints listed above.
However, this change is limited to the wp_template
and wp_template_parts
post types to mitigate the risk of introducing new bugs.
@spacedmonkey commented on PR #5194:
3 weeks ago
#14
Why register late? Why not register all post type controller like this?
#15
@
3 weeks ago
@spacedmonkey
Why register late?
The root cause of the issue is that the WP_REST_Templates_Controller::get_item()
method is incorrectly matched as the callback method to handle revisions and autosave requests for the wp_template
and wp_template_part
post types.
Do we agree on this?
If so, the solution would be to register the routes defined in both the WP_REST_Revisions_Controller and WP_REST_Autosaves_Controller controllers first.
Then, routes defined in WP_REST_Templates_Controller should be registered afterward.
This ensures that WP_REST_Server::match_request_to_handler()
matches correct routes first.
By doing this, when making a request like wp/v2/template-parts/1480/revisions
, the correct request handler is called.
I hope this explanation makes sense.
I provided a more detailed explanation in my previous comment.
Why not register all post type controllers like this?
The ability to arbitrarily specify custom post type controllers for post types can be useful.
However, it's unclear to me how this would address the route conflict mentioned earlier.
My PR should resolve the issue, and it seems no additional PRs are necessary.
I will be looking forward to hearing your feedback.
#16
@
2 weeks ago
Test Report
This report confirms that the specified PR addresses the issue.
PR tested: https://github.com/WordPress/wordpress-develop/pull/5194
Environment
- WordPress: 6.4-alpha-56267-src
- PHP: 7.3.33
- Server: Apache/2.4.57 (Unix) PHP/7.3.33
- Database: mysqli (Server: 5.7.43)
- Browser: Safari 16.6 (macOS)
- Theme: Twenty Twenty-Three 1.2
- MU-Plugins: None activated
- Plugins:
- JSON Basic Authentication 0.1
Steps to Reproduce
- Navigate to Appearance -> Editor.
- Create a new template part (also known as "pattern").
- Save the template part multiple times, making slight modifications to the template before each save. For instance, add new blocks or alter the text.
- Identify the ID of your template part in the
wp_posts
table. You can use the following SQL query to find the ID of the template part you just created:SELECT ID FROM wp_posts WHERE post_type = 'wp_template_part' ORDER BY post_date DESC LIMIT 1;
- Record the ID value.
- Install and activate the https://github.com/WP-API/Basic-Auth plugin to simplify the authentication of REST requests.
- Make a REST request to the template parts endpoint. Replace
admin:password
with your admin credentials and<template_part_id>
with the ID value from step 5.curl http://path.to.your.wordpress.server/wp-json/wp/v2/template-parts/<template_part_id>/revisions -u "admin:password"
- 🐞 The following response will be received:
{"code":"rest_template_not_found","message":"No templates exist with that id.","data":{"status":404}}
Actual Results
When testing the patch to ensure it functions as intended:
- ✅ A REST request should return a list of the revisions for your template part. For example:
[{"author":1,"date":"2023-09-12T12:14:30","date_gmt":"2023-09-12T12:14:30","id":10,"modified":"2023-09-12T12:14:30","modified_gmt":"2023-09-12T12:14:30","parent":7,"slug":"7-revision-v1","guid":{"rendered":"http:\/\/wordpress.test\/?p=10","raw":"http:\/\/wordpress.test\/?p=10"},"title":{"raw":"Template Part","rendered":"Template Part"},"content":{"raw":"<!-- wp:paragraph -->\n<p>A simple p. #3<\/p>\n<!-- \/wp:paragraph -->","rendered":"\n<p>A simple p. #3<\/p>\n"},"_links":{"parent":[{"href":"http:\/\/wordpress.test\/wp-json\/wp\/v2\/template-parts\/7"}]}},{"author":1,"date":"2023-09-12T12:14:21","date_gmt":"2023-09-12T12:14:21","id":9,"modified":"2023-09-12T12:14:21","modified_gmt":"2023-09-12T12:14:21","parent":7,"slug":"7-revision-v1","guid":{"rendered":"http:\/\/wordpress.test\/?p=9","raw":"http:\/\/wordpress.test\/?p=9"},"title":{"raw":"Template Part","rendered":"Template Part"},"content":{"raw":"<!-- wp:paragraph -->\n<p>A simple p. #2<\/p>\n<!-- \/wp:paragraph -->","rendered":"\n<p>A simple p. #2<\/p>\n"},"_links":{"parent":[{"href":"http:\/\/wordpress.test\/wp-json\/wp\/v2\/template-parts\/7"}]}},{"author":1,"date":"2023-09-12T12:14:08","date_gmt":"2023-09-12T12:14:08","id":8,"modified":"2023-09-12T12:14:08","modified_gmt":"2023-09-12T12:14:08","parent":7,"slug":"7-revision-v1","guid":{"rendered":"http:\/\/wordpress.test\/?p=8","raw":"http:\/\/wordpress.test\/?p=8"},"title":{"raw":"Template Part","rendered":"Template Part"},"content":{"raw":"<!-- wp:paragraph -->\n<p>A simple p.<\/p>\n<!-- \/wp:paragraph -->","rendered":"\n<p>A simple p.<\/p>\n"},"_links":{"parent":[{"href":"http:\/\/wordpress.test\/wp-json\/wp\/v2\/template-parts\/7"}]}}]
In conclusion, the proposed patch successfully addresses the issue. You can also test the templates endpoint for further verification.
#17
@
2 weeks ago
- Keywords needs-testing added
I am adding needs-testing because here we have only one test report, and it is from the patch developer :)
This ticket was mentioned in Slack in #core-test by oglekler. View the logs.
2 weeks ago
@antonvlasenko commented on PR #5194:
2 weeks ago
#19
Why register late? Why not register all post type controller like this?
I've replied in the ticket: https://core.trac.wordpress.org/ticket/56922#comment:13
Thank you.
#20
@
6 days ago
- Keywords changes-requested added
@antonvlasenko Your PR doesn't fix the problem. Let me explain.
The url pattern for template / template part is like this.
`
wp/v2/template-parts/twentytwentythreefooter
`
They do not use id based lookups. Meaning the urls are broken. This is why this needed a custom controller.
#21
@
6 days ago
Exactly! I wasn't sure if my solution is correct, so I really appreciate your input, @spacedmonkey.
I will update my PR accordingly.
#22
@
6 days ago
@antonvlasenko I have updated my PR with my thinking - See https://github.com/WordPress/wordpress-develop/pull/3533
Basically have a custom autosave / revisions controller with register different pattern urls. The PR is far from complete, but you want to use it as a base, that could work on getting a solution in core.
@antonvlasenko commented on PR #5194:
2 days ago
#23
Closed in favor of https://github.com/WordPress/wordpress-develop/pull/3533.
Trac ticket: https://core.trac.wordpress.org/ticket/56922