[wp-trac] [WordPress Trac] #58910: Reconsider wp_add_fields_to_navigation_fallback_embedded_links() location

[WordPress Trac]

#58910: Reconsider wp_add_fields_to_navigation_fallback_embedded_links() location
----------------------------+-----------------------
 Reporter:  SergeyBiryukov  |       Owner:  (none)
     Type:  defect (bug)    |      Status:  new
 Priority:  normal          |   Milestone:  6.3
Component:  Editor          |     Version:  trunk
 Severity:  normal          |  Resolution:
 Keywords:  has-patch       |     Focuses:  rest-api
----------------------------+-----------------------
Changes (by ramonopoly):

 * keywords:  has-patch has-unit-tests => has-patch


Comment:

 So, I don't believe we can't integrate
 `wp_add_fields_to_navigation_fallback_embedded_links` into
 `WP_REST_Navigation_Fallback_Controller` mainly because the hook is
 updating the response for all `wp_navigation` custom post types, and
 therefore does not ''directly'' relate to the navigation fallback.

 The controller class for the `wp_navigation` custom post type is
 `WP_REST_Posts_Controller`.

 Given this I think the hook function name
 `wp_add_fields_to_navigation_fallback_embedded_links` is a bit misleading.

 This leaves me wondering about the intention behind the hook. Should we:

 1. be only manipulating the `wp_navigation` post object returned by
 `/navigation-fallback` response object or
 2. all `wp_navigation` post objects returned by `/navigation`?

 If 1, then `WP_REST_Navigation_Fallback_Controller` could inherit from
 `WP_REST_Posts_Controller` and manipulate the schema object returned by
 `parent::get_item_schema();` - no hook required. Further to this point,
 the unit test `test_embedded_navigation_post_contains_required_fields`
 would have to be updated since it's testing the schema changes to
 `wp_navigation`.


 However given that particular test, I'm assuming that the intention was
 that all `wp_navigation` objects should be targeted.

 cc @get_dave and @scruffian to confirm this.

 If the above assumption is true, then some type of hook would be okay -
 either `rest_{$this->post_type}_item_schema` or `register_rest_field`
 (though I think the former is good enough). But then we'd need to work out
 where it could live, since it doesn't actually target navigation fallback
 exclusively. I'm not sure where would be appropriate.

 The alternative that requires no hooks, and perhaps leaves us with
 something more extensible and explicit might be to create a new rest
 controller class `WP_REST_Navigation_Controller` that overwrites
 `get_item_schema()`.

 I've tried that in https://github.com/WordPress/wordpress-
 develop/pull/4949

 Caveat: I could be wrong about all this :) Please advise.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/58910#comment:14>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform