-
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
Add support for pages and other custom post types #1276
Conversation
68bb448
to
bb1d0ca
Compare
What else does this need for a proper review? Trying to decide if we should expedite this instead of #1274. Guessing some client-side work with saving non-posts via the API client? |
@adamsilverstein what suggestions do you have to obtain the Backbone model name given the PHP-discovered |
@westonruter once we have the $rest_base we can localize that directly for the client, then filter the models to find the model that has the matching route. Something like: var modelToUse = _.filter( wp.api.models, function( model ) {
return (
model.prototype.route &&
0 === model.prototype.route.index.indexOf( restBase )
);
} ); |
@adamsilverstein here's what I've come up with, but it certainly doesn't feel very elegant: // Export REST bases for all post types.
$post_type_rest_base_mapping = array();
foreach ( get_post_types( array(), 'objects' ) as $post_type_object ) {
$rest_base = ! empty( $post_type_object->rest_base ) ? $post_type_object->rest_base : $post_type_object->name;
$post_type_rest_base_mapping[ $post_type_object->name ] = $rest_base;
}
$script = sprintf( 'wp.api.postTypeRestBaseMapping = %s;', wp_json_encode( $post_type_rest_base_mapping ) );
$script .= <<<JS
wp.api.getPostTypeModel = function( postType ) {
var route = '/' + wpApiSettings.versionString + this.postTypeRestBaseMapping[ postType ] + '/(?P<id>[\\\\d]+)';
return _.first( _.filter( wp.api.models, function( model ) {
return model.prototype.route && route === model.prototype.route.index;
} ) );
};
wp.api.getPostTypeRevisionsCollection = function( postType ) {
var route = '/' + wpApiSettings.versionString + this.postTypeRestBaseMapping[ postType ] + '/(?P<parent>[\\\\d]+)/revisions';
return _.first( _.filter( wp.api.collections, function( model ) {
return model.prototype.route && route === model.prototype.route.index;
} ) );
};
JS;
wp_add_inline_script( 'wp-api', $script ); |
Alright, with 2dbda35 the pages can now be edited and saved with revisions pulled in. It would be ideal if the WP-API Backbone client provided facilities for looking up models and collections based on a post type. The method used here feels very much like a workaround. The sidebar still says “Post Settings” but I think that it should just be changed to “Settings” for the sake of consistency across types and also for the sake of translatability since “%s Settings” is not among a registered post type's labels. Once this PR is blessed, I'll close #1274 and rebase this PR to remove the revert commit. |
@westonruter - I agree we can make this much more succinct by providing a helper function in the client to get the model by route (or what you are calling 'baseMapping' here. something like |
I didn't include the WP-API extensions in a separate JS file since I wasn't sure where to put them and I wanted confirmation on the approach. |
I opened a trac ticket to add this enhancement to the client: https://core.trac.wordpress.org/ticket/41111 |
We should probably create an issue to track this for discussion. Maybe we can keep the "Post Settings" label if we know it to be of the |
$post_type_rest_base_mapping[ $post_type_object->name ] = $rest_base; | ||
} | ||
$script = sprintf( 'wp.api.postTypeRestBaseMapping = %s;', wp_json_encode( $post_type_rest_base_mapping ) ); | ||
$script .= <<<JS |
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.
Could we add an inline comment noting that our adding to the wp.api
global here is temporary until support is added to the client proper? Maybe a link to the Trac ticket too.
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.
Wondering also if we ought to split out some of the unrelated logic here, like bootstrapping wp.api.getPostTypeModel
with all post type REST bases, or even below with getting / validating post and post type.
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.
@aduth can you explain further on what you mean by splitting out unrelated logic?
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.
Just thinking that this function is doing a lot of work, when it should be simple enqueues. Are these overrides specific to the editor? Maybe binding to anywhere wp-api
is enqueued. Or at least a separate function / file, or even separate action hook? Similarly below with retrieving and validating post to be edited.
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.
Oh, I see what you mean. The PHP function is doing a lot of work. 👍
@@ -57,7 +57,8 @@ class LastRevision extends Component { | |||
} | |||
this.setState( { loading: true } ); | |||
const postIdToLoad = this.props.postId; | |||
this.fetchRevisionsRequest = new wp.api.collections.PostRevisions( {}, { parent: postIdToLoad } ).fetch() | |||
const Collection = wp.api.getPostTypeRevisionsCollection( this.props.postType || 'post' ); |
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.
Maybe it should be the responsibility of the (or a new) selector to perform the fallback logic, if we expect it to be a common need? i.e. getCurrentPostType = ( state ) => getCurrentPost( state ).type || 'post';
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.
I think I'll just remove the || 'post'
part altogether. If the collection doesn't exist, then it should be due to revisions not being supported, and the component should render nothing.
2dbda35
to
3713a91
Compare
Rebased to remove page removal/revert. Former |
Added 6245840 to use Looks ready to merge if you agree 👍 |
@herovis Gutenberg depends on the post type being accessible via the REST API. So yeah, in your
In the future, Gutenberg will be the default editor, so yes. |
Fixes #1272.
Will be rebased once #1274 is merged (also if it is not merged).