#56922 closed defect (bug) (fixed)
Template / Template parts revision / autosave REST API are broken
Reported by: | spacedmonkey | Owned by: | antonvlasenko |
---|---|---|---|
Milestone: | 6.4 | Priority: | normal |
Severity: | normal | Version: | 4.7 |
Component: | REST API | Keywords: | has-patch has-unit-tests commit needs-dev-note |
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.
Attachments (3)
Change History (51)
This ticket was mentioned in โPR #3533 on โWordPress/wordpress-develop by โ@spacedmonkey.
2 years ago
#1
- Keywords has-patch added
#2
@
2 years 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
@
2 years 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:
21 months ago
#5
What would the autosaves / revisions controllers look like when they need to be custom for templates/template parts.
#6
@
21 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.
17 months ago
This ticket was mentioned in โSlack in #core by audrasjb. โView the logs.
16 months ago
#9
@
15 months 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
@
14 months 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
@
14 months 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.
14 months 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
@
14 months 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:
14 months ago
#14
Why register late? Why not register all post type controller like this?
#15
@
14 months 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
@
14 months 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
@
14 months 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.
14 months ago
โ@antonvlasenko commented on โPR #5194:
14 months 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
@
13 months 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
@
13 months 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
@
13 months 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:
13 months ago
#23
Closed in favor of โhttps://github.com/WordPress/wordpress-develop/pull/3533.
โ@spacedmonkey commented on โPR #3533:
13 months ago
#24
Unit tests are failing here. Let's get them fixed.
There needs to be unit tests of unhappy paths. So invalid data, permission checks for both, not logged in and users with incorrect permissions
This ticket was mentioned in โSlack in #core-restapi by spacedmonkey. โView the logs.
13 months ago
This ticket was mentioned in โSlack in #core by hellofromtonya. โView the logs.
13 months ago
#27
@
13 months ago
Left a request for more accurate documentation comments, but +1 on overall approach.
โ@spacedmonkey commented on โPR #3533:
13 months ago
#28
@kadamwhite I 100% agree, we need to do a full pass on the docs. But can be done after commit / RC stage. I will review docs again, but I don't want docs to be the reason we miss WP 6.4, this change has been punted 3 times already.
This ticket was mentioned in โSlack in #core by hellofromtonya. โView the logs.
13 months ago
This ticket was mentioned in โSlack in #core-test by hellofromtonya. โView the logs.
13 months ago
#31
@
13 months ago
Test Report
This report confirms that the specified PR addresses the issue.
PR tested: โhttps://github.com/WordPress/wordpress-develop/pull/3533
Environment
- WordPress: 6.4-beta2-56769-src
- PHP: 7.3.33
- Server: Apache/2.4.57 (Unix) PHP/7.3.33
- Database: mysqli (Server: 5.7.43)
- Browser: Safari 17.0 (macOS 13.6)
- 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 CONCAT_WS('/', (SELECT option_value FROM wp_options WHERE option_name = 'stylesheet'), post_name) AS template_part_id FROM wp_posts WHERE post_type = 'wp_template_part' AND post_status = 'publish' ORDER BY post_date DESC LIMIT 1;
- Record the template part 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 (slash included).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"}]}}]
Notes
โ ๏ธ In conclusion, the proposed patch successfully addresses the issue.
However, the controllers expect the template_id
and template_part_id
to always follow the format theme_name//<template(_part)_name
. In practice, it's not possible to retrieve a template or template part using only their template name (i.e., post_name
). The get_block_template()
function, employed by both REST controllers, presumes that the ID is divided into two parts separated by //
. The theme's name must always be included in the request alongside the template (part) name. Should we adjust the regular expressions for the REST routes to consistently expect the presence of the theme_name
?
#32
@
13 months ago
Flagging @spacedmonkey - What do you think with @antonvlasenko's observation and question (broken out and emphasis added)?
โ ๏ธ In conclusion, the proposed patch successfully addresses the issue.
However, the controllers expect the template_id and template_part_id to always follow the format theme_name<template(_part)_name. In practice, it's not possible to retrieve a template or template part using only their template name (i.e., post_name). The get_block_template() function, employed by both REST controllers, presumes that the ID is divided into two parts separated by . The theme's name must always be included in the request alongside the template (part) name.
Should we adjust the regex expressions for the REST routes to consistently expect the presence of the theme_name?
#33
@
13 months ago
It's great to see this PR moved forward! Thanks for all the effort! ๐๐ป
Test Report
PR tested: โhttps://github.com/WordPress/wordpress-develop/pull/3533
Steps to Reproduce and Test Patch
These steps are adapted from the test report provided in comment:16. Items marked with a green check โ
indicate expected results.
- Activate Twenty Twenty-Four and navigate to Appearance > Editor > Patterns > Manage all template parts and click the Add New Template Part button. Give it a name, "My Template Part", and Save. DO NOT YET MODIFY OR SAVE ADDITIONAL CHANGES TO THIS TEMPLATE PART.
- Using an HTTP request tool, such as Postman, or the basic auth plugin noted in comment:16, observe the GET response for the parent template part post, and take note of the
wp_id
value (this is the postID
):/wp-json/wp/v2/template-parts/twentytwentyfour/my-template-part
REPRODUCE
- ๐ Observe the GET response for the revisions endpoint:
/wp-json/wp/v2/template-parts/twentytwentyfour/my-template-part/revisions
- ๐ Observe the GET response for the autosaves endpoint:
/wp-json/wp/v2/template-parts/twentytwentyfour/my-template-part/autosaves
- ๐ฉน Apply PR/patch.
REVISIONS
- Observe the GET response for the revisions endpoint again (Step 3).
- Modify the new template part (any trivial change should work) and click Save to persist the changes, creating a new revision.
- Observe the GET response for revisions endpoint again (Step 3).
- Modify the template part again and save the changes, creating another revision.
- Observe the GET response for revisions endpoint again (Step 3).
- Append each of the unique
wp_id
values from the previous response into separate requests, and observe the responses, e.g./wp-json/wp/v2/template-parts/twentytwentyfour/my-template-part/revisions/1310
AUTOSAVES
- Observe the GET response for the autosaves endpoint again (Step 4).
- To mimic an autosave in the database, start with one of the
wp_id
values from Step 11 and locate its record in thewp_posts
table. Modify itspost_name
by changing "revision" to "autosave", e.g.1309-revision-v1
=>1309-autosave-v1
, and commit the change. - Observe the GET response for the autosaves endpoint again (Step 4).
The same steps above can be performed with the wp_template
post type by creating or modifying a template (Appearance > Editor > Templates).
Environment
- Hardware: MacBook Pro Apple M1 Pro
- OS: macOS 13.6
- Browser: Safari 16.6, Postman 10.18.11
- Server: nginx/1.25.2
- PHP: 7.4.33
- WordPress: 6.4-beta2-56769-src
- Theme: twentytwentyfour v1.0, twentytwentythree v1.2
- Active Plugins: none
Actual Results
REPRODUCE
- 3. โ
Reproduced 404 result for revisions endpoint, response code
rest_template_not_found
. - 4. โ
Reproduced 404 result for autosaves endpoint, response code
rest_template_not_found
.
REVISIONS
- 6. โ
Endpoint returns
[]
, an empty response (i.e. "no revisions"). - 8. โ
Endpoint returns a JSON response with
"slug": "1309-revision-v1"
(i.e. "there is one revision"). - 10. โ
Endpoint returns a JSON response with two revisions in the response (each with a unique
wp_id
value). - 11. โ
Requests for each unique
wp_id
value returns the individual revision.
AUTOSAVES
- 12. โ
Endpoint returns
[]
, an empty response (i.e. "no autosaves"). - 14. โ
Endpoint returns a JSON response with
"slug": "1309-autosave-v1"
.
Additional Information
- In Step 13 I mimicked an autosave because I'm not sure if this is supported by template/template parts at this time.
These observations are probably not blockers, but seem they should be documented or addressed in subsequent patches:
- โ ๏ธ Any numeric ID added to the autosaves endpoint results in the same response as without specifying an ID (e.g.
/autosaves
=/autosaves/1
), even if the post ID does not exist. Unsure if this is the expected behavior. - โ ๏ธ Note that requesting a revision ID from a different template part (e.g. using the
my-template-part
endpoint to request afooter
revision) returns the revision belonging to a different template part. EXPECT: The passed revision ID is checked against the template part. - โ ๏ธ The above also occurred when requesting a
wp_template
revision ID using thewp_template_part
endpoint (also tested revisions fromwp_global_styles
post type). EXPECT: The passed revision ID is checked against the post type. - โ ๏ธ When requesting a revision ID of a
page
post type, a response was returned, but it was largely nulled out (โsee this example). EXPECT: The passed revision ID is checked against the post type. - โ ๏ธ At this time it appears that
slug
values for revisions do not increment, e.g. for post ID 1309, each revision's slug is1309-revision-v1
regardless of iteration. EXPECT: The-vX
suffix should increment with each subsequent revision.
#34
@
13 months ago
@antonvlasenko @hellofromTonya I find your feedback a little confusing. The URL pattern you have documented is incorrect. The correct url pattern is for templates and template parts is the following
/wp/v2/templates/<id>
A template <id> is a mixture of theme and id. For example
wp/v2/templates/twentytwentyfourhome
A template part, always contains the theme and the name. That is how template parts have worked since they were merged into core.
The issue this ticket is trying to resolve is ids for post controller are normally id number based so.
wp/v2/templates/123
The revisions and autosave controllers guessed a pattern like this.
wp/v2/templates/123/revisions.
wp/v2/templates/123/autosaves.
However, as template / template parts the url pattern needs to look like this.
wp/v2/templates/twentytwentyfourhome/revisions.
wp/v2/templates/twentytwentyfourhome/autosaves.
We can't change the pattern of the url without break everything. This is how it work and this is why this ticket is needed.
Also, the shape of the response you have given looks wrong, here is what is looks like for me.
In summary, I don't think any of the things you documented are an issue, they are how it meant to work,
#35
follow-up:
โย 43
@
13 months ago
Thank you for testing this @ironprogrammer. Great feedback and amazing work!
Just wanted to reponse to your feedback at the end.
Any numeric ID added to the autosaves endpoint results in the same response as without specifying an ID (e.g. /autosaves = /autosaves/1), even if the post ID does not exist. Unsure if this is the expected behavior.
I was unable to replicate this error. Can provide more context.
โ ๏ธ Note that requesting a revision ID from a different template part (e.g. using the my-template-part endpoint to request a footer revision) returns the revision belonging to a different template part. EXPECT: The passed revision ID is checked against the template part.
โ ๏ธ The above also occurred when requesting a wp_template revision ID using the wp_template_part endpoint (also tested revisions from wp_global_styles post type). EXPECT: The passed revision ID is checked against the post type.
โ ๏ธ When requesting a revision ID of a page post type, a response was returned, but it was largely nulled out (see this example). EXPECT: The passed revision ID is checked against the post type.
Basically these are the same issue. The parent id of the revision is not compared original post. Sadly this is an issue with the revisions controller as well. See screen, where I am requesting a revision for post 1 and I get the a revision for from a different post. This is completely unrelated to this change. But it is a great find and I will create a bug for this after beta has died down.
โ ๏ธ At this time it appears that slug values for revisions do not increment, e.g. for post ID 1309, each revision's slug is 1309-revision-v1 regardless of iteration. EXPECT: The -vX suffix should increment with each subsequent revision.
This issue is an related to the change. This is how revisions are made, using the revisions API ( functions ) in core. They are a little weird, IMO and could do with some review. But this change should be considered, will create a follow up ticket for this idea.
TDLR, all the documented bugs are upstream issues with revisions api or the revisions / autosave controllers. These issues should be handled elsewhere in a different ticket.
This ticket was mentioned in โSlack in #core by spacedmonkey. โView the logs.
13 months ago
โ@spacedmonkey commented on โPR #3533:
13 months ago
#37
Thanks @spacedmonkey for the PR. Left some minor feedback. Do we need the changes from
tests/qunit/fixtures/wp-api-generated.js
file?
Yes, it is needed. All updates to the rest api need this update. See this example โhttps://github.com/WordPress/wordpress-develop/commit/4f56c3d18a7236048ed40bfa8c82fb8c88f9b361
#38
@
13 months ago
In summary, I don't think any of the things you documented are an issue, they are how it meant to work,
Thank you for the detailed reply, @spacedmonkey . I've read your response attentively and I agree with your explanation of how things are meant to work. However, that's not exatly the point I'm trying to make.
Please take a look at this PHP snippet: โhttps://3v4l.org/aARJS, specifically at its output at the very bottom of the page. Notice how the $current_regular_expression
matches IDs that shouldn't be matched. Conversely, $fixed_regular_expression
works as intended, not matching the incorrect IDs.
To fix the issue, (?:\/[^\/:<>\*\?"\|]+)?
should be replaced with \/\/?[^\/:<>\*\?"\|]+
in all regular expressions defining the routes in the new controllers, because the group that matches the template ID (post_name
) part of the ID should not be optional.
I've added these changes as code review suggestions for your consideration.
#39
@
13 months ago
I've updated the PHP snippet above and the code review suggestions.
My previous changes didn't correctly match requests with double slashes, but this issue should be resolved now.
I apologize for the confusion.
#40
@
13 months ago
- Keywords needs-testing changes-requested removed
Bringing โmy comments over from Make/Core slack:
What about committing it?
I have concerns of committing a patch with this amount of changes, especially one that introduces new public properties and methods. This is why I asked for additional test reports.
The test reports revealed some observations, some of which will require follow-up in new tickets (some revealed issues upstream as Jonny noted).
The PR has gone through a thorough review with approvals from a REST API Maintainer @kadamwhite and Committers including @kadamwhite and @costdev. There are multiple test reports showing it works as expected including the ones from yesterday from @antonvlasenko and @ironprogrammer.
IMO a big change like this being committed in the last beta release is concerning, but the risk concerns are mitigated enough by all of the above. And they can be further validated in Beta 3 and even into RC, should a follow-up or revert be required.
I think it's a green light :large_green_circle: to proceed with the commit.
Leaving the commit
decision to those who are deeply involved in its development and approval process.
#41
@
13 months ago
- Keywords commit needs-dev-note added
- Owner set to spacedmonkey
- Status changed from new to assigned
Assigning to myself to commit
โ@spacedmonkey commented on โPR #3533:
13 months ago
#42
#43
in reply to:
โย 35
@
13 months ago
Replying to spacedmonkey:
Any numeric ID added to the autosaves endpoint results in the same response as without specifying an ID (e.g. /autosaves = /autosaves/1), even if the post ID does not exist. Unsure if this is the expected behavior.
I was unable to replicate this error. Can provide more context.
To replicate this, make sure there exists at least one autosave for the template in question. E.g. in your example, modify the twentytwentyfour/home
template to create a revision, then in the DB change the revision's post_name
to replace "revision" with "autosave" (like 1234-autosave-v1
). Then it shouldn't matter what numeric ID you supply in the request, it will always return that autosave.
And thank you for reviewing the other noted items! It's good to know where these refinements should be chased down.
#44
@
13 months ago
- Owner changed from spacedmonkey to antonvlasenko
I will have limited capability to response to this ticket in the next couple of days. I am assigning to @antonvlasenko who worked on the patch, to review and follow up issues.
#45
@
13 months ago
If I recall correctly (big if :)), this ticket is ready to be closed as fixed and follow-up work for the noted adjacent finding and possible enhancements could continue in separate tickets during 6.5.
@antonvlasenko am I recalling correct? Can this ticket be closed as fixed? Or are there additional items to address ahead of 6.4 RC1?
#46
@
13 months ago
Or are there additional items to address ahead of 6.4 RC1?
@hellofromTonya
Some enhancements might be needed for the regular expressions that define the new endpoints introduced in โhttps://github.com/WordPress/wordpress-develop/pull/3533. Also, certain inline comments need to be fixed too.
For example:
* Matches theme's directory: `/themes/<subdirectory>/<theme>/` or `/themes/<theme>/`.
This inline comment is not entirely accurate, as the actual URLs do not include /themes
, etc.
However, I believe these changes could be addressed in a separate Trac ticket as none of these issues are major.
If there are concerns that they should be included in this ticket for contextual or historical reasons, then let's keep it open.
#47
follow-up:
โย 48
@
13 months ago
- Resolution set to fixed
- Status changed from assigned to closed
Thank you @antonvlasenko. Please open a new ticket for those improvements. Would be good to drop a comment here with that new ticket number.
I'm closing this ticket as fixed. Thank you everyone for your contributions :)
#48
in reply to:
โย 47
@
13 months ago
Replying to hellofromTonya:
Thank you @antonvlasenko. Please open a new ticket for those improvements. Would be good to drop a comment here with that new ticket number.
Done.
Follow-up ticket: https://core.trac.wordpress.org/ticket/59635.
Trac ticket: https://core.trac.wordpress.org/ticket/56922