-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Implement core style of including revisions data on Post response #7495
Conversation
lib/rest-api.php
Outdated
$new_links = array(); | ||
$orig_links = $response->get_links(); | ||
|
||
if ( in_array( $post->post_type, array( 'post', 'page' ), true ) || post_type_supports( $post->post_type, 'revisions' ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we hardcode support for post
and page
types, rather than letting it follow the same logic of using post_type_supports
(since, by default, they do register support)? If it's needed, can we leave an inline comment explaining why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we hardcode support for
post
andpage
types, rather than letting it follow the same logic of usingpost_type_supports
(since, by default, they do register support)?
To "force" Posts and Pages to include the revisions endpoint (even if a developer has removed support via post_type_supports
). This was a decision made in the original API design. The conditional is copied directly.
However, this is moot with 874d8b7
lib/rest-api.php
Outdated
$revisions_count = count( $revisions ); | ||
|
||
$new_links['version-history'] = array( | ||
'href' => $orig_href . '/revisions', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a reliable way to generate a revisions API URL ? Could we use WP_REST_Response::get_links
to retrieve the original links before removing version-history
and just alter (even via remove_link
+ add_link
) the original value to include the count
field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, updated in 874d8b7
lib/rest-api.php
Outdated
if ( $revisions_count > 0 ) { | ||
$last_revision = array_shift( $revisions ); | ||
|
||
$new_links['predecessor'] = array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per this comment and the referenced RFC, should this be predecessor-version
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be
predecessor-version
Yes, corrected in 9823c74
c5075ee
to
9823c74
Compare
'count' => $revisions_count, | ||
); | ||
|
||
if ( $revisions_count > 0 ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be greater than 1? I think we don't render the link if there is only 1 revision.
Yep, confirmed:
if ( ! lastRevisionId || revisionsCount < 2 ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be greater than 1? I think we don't render the link if there is only 1 revision.
That's a client specific detail. Seems to me the REST API should provide the raw value (maybe even if it were 0
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming it is still a correct url, is fine as is.
$revisions = wp_get_post_revisions( $post->ID, array( 'fields' => 'ids' ) ); | ||
$revisions_count = count( $revisions ); | ||
|
||
$new_links['version-history'] = array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: If intent is to merge in the count
attribute, may be more clear if used array_merge
to indicate as such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$orig_links['version-history']
has a different format and can't be used directly.
* 'master' of https://github.com/WordPress/gutenberg: (69 commits) fix: Show permalink editor in editor (WordPress#7494) Fix text wrapping in Firefox. (WordPress#7472) Try another approach to fixing the sibling inserter in Firefox (WordPress#7530) fix: Improve "add block" text in NUX onboarding (WordPress#7511) Implement core style of including revisions data on Post response (WordPress#7495) Testing: Add e2e test for PluginPostStatusInfo (WordPress#7284) Add end 2 end test for sidebar behaviours on mobile and desktop. (WordPress#6877) Only save metaboxes when it's not an autosave (WordPress#7502) Fix broken links in documentation (WordPress#7532) Remove post type 'viewable' compatibility shim (WordPress#7496) Fix typo. (WordPress#7528) Blocks: Remove wrapping div from paragraph block (WordPress#7477) fix: change import for InnerBlocks (WordPress#7484) Polish library just a teeeeensy bit (WordPress#7522) feat: Add snapshot update script (WordPress#7514) Display server error message when one exists (WordPress#7434) Fix issues with gallery in IE11. (WordPress#7465) Polish region focus style (WordPress#7459) Fix IE11 formatting toolbar visibility (WordPress#7413) Update plugin version to 3.1. (WordPress#7402) ...
Description
Incorporates core pattern of including revisions data on Post response
See https://core.trac.wordpress.org/ticket/44321#comment:9
Fixes #3258
How has this been tested?
Checklist: