REST API: Simplify 'default_rendering_mode' field registration for post types #67867
+42
−66
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mamaduka
added
REST API Interaction
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Mamaduka
commented
Dec 12, 2024
lib/compat/wordpress-6.8/class-gutenberg-rest-post-types-controller-6-8.php
Outdated
Show resolved
Hide resolved
swissspidy
reviewed
Dec 12, 2024
| */ | ||
| $rendering_mode = apply_filters( "post_type_{$item->name}_default_rendering_mode", $rendering_mode, $item ); | ||
| // Property will only exist if the post type supports the block editor. | ||
| if ( ! $post_type_object || ! isset( $post_type_object->default_rendering_mode ) ) { |
There was a problem hiding this comment.
Still not a fan of a) artificially adding properties to the WP_Post_Type and b) the name
There was a problem hiding this comment.
What do you mean artificially? This is the only way to expose new properties in response.
Agreed on the name, I'll create a new discussion for it + if we should migrate to supports. This PR here is just refactoring.
There was a problem hiding this comment.
What do you mean
artificially? This is the only way to expose new properties in response.
When I worked on WP_Post_Type it wasn't really the intention to allow passing non-default fields to post type registration.
There was a problem hiding this comment.
I like your posed idea in the original PR to use post type supports - 'editor-rendering-mode' => 'post-only', and I guess we can make that change here.
It will be a conditional support value, like the autosave you recently implemented.
@fabiankaegy, @TylerB24890, @youknowriad, what do you think?
Edit: I think the key difference compared to other support values is that you can't really remove it. Removal would mean fallback to default rendering mode - post-only.
There was a problem hiding this comment.
In my mind post type supports should be for actual supports. Not for setting defaults.
Even if a post type doesn't support this, the user can still get into this mode by simple selecting the "Show Template" option that we surface in two places in the UI.
That is a very weakly held opinion and if you all think it should be a support I won't block that. I just want to be clear that removing the support unlike virtually every other support that I know of doesn't actually remove that feature, It just doesn't have the option in the UI checked by default. But the feature is still very much there
There was a problem hiding this comment.
It doesn't have to be a new support, it can be a configuration option of the editor support.
There was a problem hiding this comment.
I do like the idea of that!
Mamaduka
commented
Dec 12, 2024
| return { | ||
| hasLoadedPostObject: _hasLoadedPostObject, | ||
| editorSettings: getEditorSettings(), | ||
| isReady: __unstableIsEditorReady(), | ||
| mode: getRenderingMode(), | ||
| defaultMode: | ||
| postTypeObject?.default_rendering_mode ?? 'post-only', | ||
| postTypeObject?.default_rendering_mode || 'post-only', |
There was a problem hiding this comment.
Just extra hardening to provide correct fallback on falsy values like empty string
|
Size Change: -44 B (0%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
Mamaduka
force-pushed
the
update/rest-post-type-rendering-mode
branch
from
3f047eb to
547a7cc
Compare
This was referenced Dec 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
A follow-up to #62304.
Simplifies the registration of the
default_rendering_modepost-type REST property by adding it viaregister_rest_fieldinstead of a controller override. It also adds a schema for the new property.I've removed the new filters introduced in the original PR. Why:
default_rendering_modefor REST API responses. This could cause some unexpected results.register_post_type_argsorrest_prepare_post_typefilters.In the future, we could provide API like
add_post_type_support, but first, let's see how these configurations mature.cc @TylerB24890
Testing Instructions
template-lockedmode.Testing Instructions for Keyboard
Same.