Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for custom revision / autosave endpoints classes. #3533

Closed

Conversation

spacedmonkey
Copy link
Member

@spacedmonkey spacedmonkey commented Oct 27, 2022

TODO

  • Unit tests for Post type class.
  • Unit tests for template revisions rest api endpoint
  • Unit tests for template autosave rest api endpoint

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

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@TimothyBJacobs
Copy link
Member

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

Copy link
Contributor

@kadamwhite kadamwhite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Takes a lot of code to do this, doesn't it! Thanks for fleshing out the unit tests more, @spacedmonkey .

I am fine with the overall approach, and mostly want to encourage a full once-over of all docblocks to make sure they're not strictly copy-pasted from the base controller when more specific wording might help us in the future.

@spacedmonkey
Copy link
Member Author

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

spacedmonkey and others added 3 commits October 9, 2023 13:58
Co-authored-by: K Adam White <kadamwhite@users.noreply.github.com>
Copy link
Contributor

@costdev costdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @spacedmonkey!

Some minor feedback then this is good from my perspective. 🙂

spacedmonkey and others added 3 commits October 9, 2023 15:13
Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
Copy link
Contributor

@costdev costdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the ! empty() conversation and these two typo corrections left. Pre-approving as I'll be AFK for a bit. 🙂

Copy link
Contributor

@kadamwhite kadamwhite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, @spacedmonkey , so long as we do come back to the docs before release. Thank you for addressing the top issues I flagged—approving from my end to unblock moving forward.

spacedmonkey and others added 4 commits October 9, 2023 21:55
….php

Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
….php

Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
….php

Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @spacedmonkey for the PR. Left some minor feedback. Do we need the changes from tests/qunit/fixtures/wp-api-generated.js file?

src/wp-includes/block-template-utils.php Outdated Show resolved Hide resolved
src/wp-includes/class-wp-post-type.php Outdated Show resolved Hide resolved
src/wp-includes/post.php Outdated Show resolved Hide resolved
tests/phpunit/tests/post/wpPostType.php Outdated Show resolved Hide resolved
tests/phpunit/tests/post/wpPostType.php Outdated Show resolved Hide resolved
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
@spacedmonkey
Copy link
Member Author

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 4f56c3d

Copy link

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explanation of why these changes are needed:

Additionally, the rest-schema-setup.php and wp-api-generated.js files will need to be updated too.

* Matches theme's directory: `/themes/<subdirectory>/<theme>/` or `/themes/<theme>/`.
* Excludes invalid directory name characters: `/:<>*?"|`.
*/
'([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)',
'([^\/:<>\*\?"\|]+\/\/?[^\/:<>\*\?"\|]+)',

* Matches theme's directory: `/themes/<subdirectory>/<theme>/` or `/themes/<theme>/`.
* Excludes invalid directory name characters: `/:<>*?"|`.
*/
'([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)',
'([^\/:<>\*\?"\|]+\/\/?[^\/:<>\*\?"\|]+)',

* Matches theme's directory: `/themes/<subdirectory>/<theme>/` or `/themes/<theme>/`.
* Excludes invalid directory name characters: `/:<>*?"|`.
*/
'([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)',
'([^\/:<>\*\?"\|]+\/\/?[^\/:<>\*\?"\|]+)',

* Matches theme's directory: `/themes/<subdirectory>/<theme>/` or `/themes/<theme>/`.
* Excludes invalid directory name characters: `/:<>*?"|`.
*/
'([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)',
'([^\/:<>\*\?"\|]+\/\/?[^\/:<>\*\?"\|]+)',

public function test_register_routes() {
$routes = rest_get_server()->get_routes();
$this->assertArrayHasKey(
'/wp/v2/templates/(?P<id>([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)[\/\w%-]+)/autosaves',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'/wp/v2/templates/(?P<id>([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)[\/\w%-]+)/autosaves',
'/wp/v2/templates/(?P<id>([^\/:<>\*\?"\|]+\/\/?[^\/:<>\*\?"\|]+)[\/\w%-]+)/autosaves',

'Template part autosaves route does not exist.'
);
$this->assertArrayHasKey(
'/wp/v2/template-parts/(?P<parent>([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)[\/\w%-]+)/autosaves/(?P<id>[\d]+)',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'/wp/v2/template-parts/(?P<parent>([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)[\/\w%-]+)/autosaves/(?P<id>[\d]+)',
'/wp/v2/template-parts/(?P<parent>([^\/:<>\*\?"\|]+\/\/?[^\/:<>\*\?"\|]+)[\/\w%-]+)/autosaves/(?P<id>[\d]+)',

public function test_register_routes() {
$routes = rest_get_server()->get_routes();
$this->assertArrayHasKey(
'/wp/v2/templates/(?P<parent>([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)[\/\w%-]+)/revisions',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'/wp/v2/templates/(?P<parent>([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)[\/\w%-]+)/revisions',
'/wp/v2/templates/(?P<parent>([^\/:<>\*\?"\|]+\/\/?[^\/:<>\*\?"\|]+)[\/\w%-]+)/revisions',

'Template revisions route does not exist.'
);
$this->assertArrayHasKey(
'/wp/v2/templates/(?P<parent>([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)[\/\w%-]+)/revisions/(?P<id>[\d]+)',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'/wp/v2/templates/(?P<parent>([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)[\/\w%-]+)/revisions/(?P<id>[\d]+)',
'/wp/v2/templates/(?P<parent>([^\/:<>\*\?"\|]+\/\/?[^\/:<>\*\?"\|]+)[\/\w%-]+)/revisions/(?P<id>[\d]+)',

'Single template revision based on the given ID route does not exist.'
);
$this->assertArrayHasKey(
'/wp/v2/template-parts/(?P<parent>([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)[\/\w%-]+)/revisions',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'/wp/v2/template-parts/(?P<parent>([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)[\/\w%-]+)/revisions',
'/wp/v2/template-parts/(?P<parent>([^\/:<>\*\?"\|]+\/\/?[^\/:<>\*\?"\|]+)[\/\w%-]+)/revisions',

'Template part revisions route does not exist.'
);
$this->assertArrayHasKey(
'/wp/v2/template-parts/(?P<parent>([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)[\/\w%-]+)/revisions/(?P<id>[\d]+)',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'/wp/v2/template-parts/(?P<parent>([^\/:<>\*\?"\|]+(?:\/[^\/:<>\*\?"\|]+)?)[\/\w%-]+)/revisions/(?P<id>[\d]+)',
'/wp/v2/template-parts/(?P<parent>([^\/:<>\*\?"\|]+\/\/?[^\/:<>\*\?"\|]+)[\/\w%-]+)/revisions/(?P<id>[\d]+)',

@spacedmonkey
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants