Template / Template parts revision / autosave REST API are broken

[spacedmonkey]

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.

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

[spacedmonkey]

I have made a start on a solution that implements custom autosave / revision endpoints.

[revgeorge]

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.

[spacedmonkey]

@TimothyBlynJacobs Love your thoughts on this.

What would the autosaves / revisions controllers look like when they need to be custom for templates/template parts.

[andraganescu]

Hello! Is this still something that has a chance to land on 6.2? :) I know it's really late.

[hellofromTonya]

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.

[antonvlasenko]

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.

[spacedmonkey]

@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.

### 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.

[antonvlasenko]

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.

Why register late? Why not register all post type controller like this?

[antonvlasenko]

@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.

[antonvlasenko]

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

1. Navigate to Appearance -> Editor.
2. Create a new template part (also known as "pattern").
3. Save the template part multiple times, making slight modifications to the template before each save. For instance, add new blocks or alter the text.
4. 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;
   

5. Record the ID value.
6. Install and activate the ​https://github.com/WP-API/Basic-Auth plugin to simplify the authentication of REST requests.
7. 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.

    bash
   curl http://path.to.your.wordpress.server/wp-json/wp/v2/template-parts/<template_part_id>/revisions -u "admin:password"
   

8. 🐞 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.

[oglekler]

I am adding needs-testing because here we have only one test report, and it is from the patch developer :)

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.

[spacedmonkey]

@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.

[antonvlasenko]

Exactly! I wasn't sure if my solution is correct, so I really appreciate your input, @spacedmonkey.
I will update my PR accordingly.

[spacedmonkey]

@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.

Closed in favor of ​https://github.com/WordPress/wordpress-develop/pull/3533.

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

[kadamwhite]

Left a request for more accurate documentation comments, but +1 on overall approach.

@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.

[antonvlasenko]

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

1. Navigate to Appearance -> Editor.
2. Create a new template part (also known as "pattern").
3. Save the template part multiple times, making slight modifications to the template before each save. For instance, add new blocks or alter the text.
4. 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;
   

5. Record the template part ID value.
6. Install and activate the ​https://github.com/WP-API/Basic-Auth plugin to simplify the authentication of REST requests.
7. 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).

    bash
   curl http://path.to.your.wordpress.server/wp-json/wp/v2/template-parts/<template_part_id>/revisions -u "admin:password"
   

8. 🐞 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?

[hellofromTonya]

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?

[ironprogrammer]

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.

1. 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.
2. 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 post ID):

   /wp-json/wp/v2/template-parts/twentytwentyfour/my-template-part
   

REPRODUCE

3. 🐛 Observe the GET response for the revisions endpoint:

   /wp-json/wp/v2/template-parts/twentytwentyfour/my-template-part/revisions
   

4. 🐛 Observe the GET response for the autosaves endpoint:

   /wp-json/wp/v2/template-parts/twentytwentyfour/my-template-part/autosaves
   

5. 🩹 Apply PR/patch.

REVISIONS

6. Observe the GET response for the revisions endpoint again (Step 3).
7. Modify the new template part (any trivial change should work) and click Save to persist the changes, creating a new revision.
8. Observe the GET response for revisions endpoint again (Step 3).
9. Modify the template part again and save the changes, creating another revision.
10. Observe the GET response for revisions endpoint again (Step 3).
11. 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

12. Observe the GET response for the autosaves endpoint again (Step 4).
13. To mimic an autosave in the database, start with one of the wp_id values from Step 11 and locate its record in the wp_posts table. Modify its post_name by changing "revision" to "autosave", e.g. 1309-revision-v1 => 1309-autosave-v1, and commit the change.
14. 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 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.
• ⚠️ 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.

[spacedmonkey]

@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,

[spacedmonkey]

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.

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

[antonvlasenko]

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.

[antonvlasenko]

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.

[hellofromTonya]

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.

[spacedmonkey]

Assigning to myself to commit

Committed https://core.trac.wordpress.org/changeset/56819

[ironprogrammer]

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.

[spacedmonkey]

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.

[hellofromTonya]

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?

[antonvlasenko]

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.

[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.

I'm closing this ticket as fixed. Thank you everyone for your contributions :)

[antonvlasenko]

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.