-
Notifications
You must be signed in to change notification settings - Fork 34
REST API: Add source
parameter for types endpoint
#128
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
base: trunk
Are you sure you want to change the base?
Conversation
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.
In my opinion, most of console logs are not necessary. Let's avoid to fill the logs of CI with them. The rest of the PR is working as expected, nice job!! 🚀
switch ( $source ) { | ||
case 'core': | ||
$result = $core_types; | ||
break; | ||
case 'scf': | ||
$result = $scf_types; | ||
break; | ||
case 'other': | ||
$result = array_diff( | ||
array_keys( get_post_types( array(), 'objects' ) ), | ||
array_merge( $core_types, $scf_types ) | ||
); | ||
break; | ||
default: | ||
$result = array(); | ||
} | ||
|
||
return $result; |
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.
NIT: We can return on each case.
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 like this too; why not? :)
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.
Nice work!
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 didn't comb through every little detail, but overall seems pretty good! I just left a concern about caching.
if ( null === $this->cached_post_types ) { | ||
$this->cached_post_types = $this->get_source_post_types( $source ); | ||
} | ||
$source_post_types = $this->cached_post_types; |
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.
In practice, the way the REST API works is that one request corresponds to one fresh execution of the WP stack, including running through SCF_Rest_Types_Endpoint
's constructor and down that stack. However, semantically, cached_post_types
is an instance property of a class that represents an endpoint, not a request. Here's my concern when I read this chunk:
Is there any scenario (e.g. request batching or sequential rest_do_request
calls) where it's possible to process more than one request with the same endpoint instance? If so, this cache will need invalidation. At the very least, it should be source-indexed:
$source_post_types = $this->cached_post_types[ $source ];
… but ideally its lifetime should be that of a request.
No?
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.
You are right! Instead of source-indexing it, which was something I considered from the start and wanted to avoid, I've opted for a cleaner solution in 555217b: clear the cache at the end of each request, so that it is truly stateless.
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.
555217b
to
9003531
Compare
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.
This looks much much better. Left some notes, still. Also, don't rely on my approval for merging! You all have the domain knowledge, not me. :)
* @return array The filtered response data. | ||
*/ | ||
public function clean_types_response( $response, $server, $request ) { | ||
if ( strpos( $request->get_route(), '/wp/v2/types' ) !== 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.
This match is too eager, catching any word starting with types
. Could be a regex pattern similar to one already used in this diff: preg_match( '#^/wp/v2/types/([^/]+)$#'
. Drive-by questions:
- is there a REST utility for this?
- can we assume a trailing slash will always be there, or do we need to account for
/types$/
?
$all_post_types = get_post_types( array( '_builtin' => true ), 'objects' ); | ||
foreach ( $all_post_types as $post_type ) { | ||
$core_types[] = $post_type->name; | ||
} |
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.
There's wp_list_pluck
, though I don't know if it performs worse. :)
Ideally, there would be array_column
, but AFAIK that operates on items of type array, not object.
(This is an FYI, no need to change unless you'd prefer to.)
if ( function_exists( 'acf_get_internal_post_type_posts' ) ) { | ||
$scf_managed_post_types = acf_get_internal_post_type_posts( 'acf-post-type' ); | ||
foreach ( $scf_managed_post_types as $scf_post_type ) { | ||
if ( isset( $scf_post_type['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.
Asking as SCF-ignorant: when would there be no post_type
in an item in a collection returned by a function named acf_get_internal_post_type_posts_?
switch ( $source ) { | ||
case 'core': | ||
$result = $core_types; | ||
break; | ||
case 'scf': | ||
$result = $scf_types; | ||
break; | ||
case 'other': | ||
$result = array_diff( | ||
array_keys( get_post_types( array(), 'objects' ) ), | ||
array_merge( $core_types, $scf_types ) | ||
); | ||
break; | ||
default: | ||
$result = array(); | ||
} | ||
|
||
return $result; |
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 like this too; why not? :)
// Not needed for OpenAPI documentation | ||
if ( $include_validation ) { |
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 it common practice to explicitly remove them? In other words, is the small complexity cost of this branching worth the benefit of not having these params in the route map? Also, if these params aren't needed and are undesirable, why doesn't the REST framework strip them for all?
What
/wp/v2/types
REST API endpoint that allows filtering post types by their origin:core
(WordPress built-in),scf
(for SCF managed types), orother
for the rest of CPTs./wp/v2/types
) and single item (/wp/v2/types/{type}
) endpointsWhy
When working on the Command Palette Integration and considering using the REST API to define CPT-related commands, requesting specifically for the CPTs managed by SCF became a need, and it will be needed, too, when implementing DataViews.
In fact, this branch is directly based on #124 and started in parallel, as we both realized we needed to tweak the Types endpoint if we wanted to leverage the API.
How
source
parameter is not defined, nothing changes.rest_request_before_callbacks
for the single request, returning 404 if the requested type doesn't meet the criteria.rest_prepare_post_type
to filter each post type by source before WordPress prepares it.rest_pre_echo_response
to removenull
values returned byrest_prepare_post_type
in the response array.rest_prepare_post_type
fires once per post type, the Post Types belonging to the requested source are calculated in advance inrest_request_before_callbacks
and reused throughout the whole request.Testing Instructions
- Make a GET request to
/wp/v2/types
to see all post types- Add
?source=core
to filter for WordPress built-in post types only- Add
?source=scf
to see only post types managed by SCF- Try
?source=other
to see post types registered from other sources- Test single post type endpoints like
/wp/v2/types/post?source=core
- Try an invalid source value (e.g.,
?source=invalid
) to verify validation works- Confirm a 404 error when requesting a post type with the wrong source (e.g., /wp/v2/types/post?source=scf`
- Run PHPUnit tests:
composer test:php -- --filter=Test_REST_Types_Endpoint
- Run E2E tests:
npm run test:e2e tests/e2e/rest-api-types-endpoint.spec.ts
Potential follow-ups