Reconsider wp_add_fields_to_navigation_fallback_embedded_links() location

[SergeyBiryukov]

[56052] / #58557 added the wp-includes/navigation-fallback.php file with a single function,
wp_add_fields_to_navigation_fallback_embedded_links().

At a glance, adding a separate file for only one function like that is not something that WP core generally does. In a discussion with @aristath, it looks like this was directly copied from the Gutenberg plugin, and might need to be rearchitected properly for core. The add_filter() call should probably be moved to default-filters.php, and there might be a more appropriate location for the function itself as well.

I wonder if that function should instead be a method of the WP_REST_Navigation_Fallback_Controller class, or if register_rest_field() should be used for those extra properties, so that ::add_additional_fields_schema() works for them.

[swissspidy]

I wonder if that function should instead be a method of the WP_REST_Navigation_Fallback_Controller class, or if register_rest_field() should be used for those extra properties, so that ::add_additional_fields_schema() works for them.

Big +1. It really does not make sense as a standalone function in core.

Making the wp_add_fields_to_navigation_fallback_embedded_links() a method of WP_REST_Navigation_Fallback_Controller class

Trac ticket: https://core.trac.wordpress.org/ticket/58910

[rajinsharwar]

Hi @SergeyBiryukov, I tried to implement your suggestion of Making the wp_add_fields_to_navigation_fallback_embedded_links() a method of WP_REST_Navigation_Fallback_Controller class.

[afercia]

+1 One more concern, if I recall correctly, is that historically WordPress has always been very cautious with introducing new files as there's always some risk that writing on the filesystem goes wrong during updates.

I fixed the docblock issue in ​ce29cf0 FYI

[audrasjb]

@SergeyBiryukov do you plan on committing this change? Thinking that before RC3 would be ideal, as we're now very close to the final release. Otherwise, I'm also fine with punting this to 6.3.1 :)

[kirasong]

I like the idea of avoiding adding a file as well, unless there are reasons this is an issue.

cc: Some folks that worked on this that aren't already in the discussion -- please feel free to ping additional folks as well:

@isabel_brison @ramonopoly @get_dave @scruffian @getdave

[SergeyBiryukov]

Replying to audrasjb:

do you plan on committing this change?

I think this could use some more testing.

I'm not familiar with the navigation fallback code yet, specifically how the WP_REST_Navigation_Fallback_Controller class relates to WP_REST_Posts_Controller, where the rest_{$this->post_type}_item_schema filter (utilized here) is applied.

I would like to take a closer look at all the interactions here later today, but most likely after 6.3 RC3.

[isabel_brison]

I like the idea of avoiding adding a file as well, unless there are reasons this is an issue.

I don't believe so! It should be fine to move to an existing file.

Probably best aim for 6.3.1 at this point :)

[ramonopoly]

+1 to integrating it into existing Core classes/files

[JeffPaul]

Given that this will be introduced with 6.3, it would be ideal to resolve before final release next week as waiting for 6.3.1 could lead to folks relying on the current location/approach. I know that time is short, but if there's capacity to re-architect into an existing file that seems much better than punting a newly introduced issue.

[ramonopoly]

I can try to get something started today. I might have to ask @get_dave to validate/continue if I can't finish by this evening (AEST).

This PR is an attempt to resolve the concerns in https://core.trac.wordpress.org/ticket/58910: a hook (wp_add_fields_to_navigation_fallback_embedded_links) was inserted via rest_{$this->post_type}_item_schema to manipulate the wp_navigation post object rest response. It was added to a new file in Core and also referred to "navigation fallback", when in fact it was being applied to all wp_navigation post types.

The strategy is to create a new rest API class - WP_REST_Navigation_Controller - to overwrite the get_item_schema method of WP_REST_Posts_Controller.

wp_add_fields_to_navigation_fallback_embedded_links and the file it lives in has been deleted. The unit test is now covered in WP_REST_Navigation_Fallback_Controller_Test.

Tests have been added for this class.

Trac ticket:

[ramonopoly]

So, I 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 via rest_wp_navigation_item_schema, 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 embedded link response object or
2. all wp_navigation post objects returned by /navigation?

Given the contents of the test_embedded_navigation_post_contains_required_fields 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.

This was the original approach we wanted to take. See ​https://github.com/WordPress/gutenberg/pull/50032

Initially I was returning the full Navigation post object from the REST endpoint. However, this required the introduction of a endpoint schema that married with the full Post object.

I spoke to Core REST folks about this and they advised that as we'd (collectively) agreed not to extend the Posts Controller, the most suitable option was simply to return a reference to the Navigation Post (i.e. the id) and then include a self link within the response which could optionally be embedded.

This resulted in a more manageable schema whilst also retaining the ability to access the full post object as part of the request if necessary.

This was the original approach we wanted to take. See ​https://github.com/WordPress/gutenberg/pull/50032

Initially I was returning the full Navigation post object from the REST endpoint. However, this required the introduction of a endpoint schema that married with the full Post object.

I spoke to Core REST folks about this and they advised that as we'd (collectively) agreed not to extend the Posts Controller, the most suitable option was simply to return a reference to the Navigation Post (i.e. the id) and then include a self link within the response which could optionally be embedded.

This resulted in a more manageable schema whilst also retaining the ability to access the full post object as part of the request if necessary.

[johnjamesjacoby]

I really like ​https://github.com/WordPress/wordpress-develop/pull/4949 – it seems, to me, to be the right way to completely polish-off this feature 💯

However... there is a non-zero amount of irony in the replacing "a new file with a single function" with "a new file with a single class" – I understand how they are different, but can't resist chuckling at how they are the same 🤣

[ramonopoly]

This was the original approach we wanted to take

Thanks for digging up the context @scruffian it’s really helpful.

I’m happy with either way. If we keep the hook, maybe we should rename it as it seems to affect all the wp_navigation post objects and not just the one returned by the fallback embedded link?

Then I guess we should answer whether to create a new file and where it should live 👍

there is a non-zero amount of irony in the replacing "a new file with a single function"

The irony isn’t lost on me :-) but what would life be without it 🤷‍♀️ha

[ramonopoly]

Then I guess we should answer whether to create a new file and where it should live 👍

If we don't want to extend WP_REST_Posts_Controller, what about adding the hook as a public method of WP_Navigation_Fallback?

add_filter( 'rest_wp_navigation_item_schema', array( 'WP_Navigation_Fallback', 'update_wp_navigation_post_schema' ), 22 );

Nothing else would need to change, except for maybe the comments and function name, which I still consider a bit misleading. That is, the effect is for all wp_navigation post types but used specifically in WP_REST_Navigation_Fallback_Controller.

Alternative here: ​https://github.com/WordPress/wordpress-develop/pull/4957

Alternative to:

• ​https://github.com/WordPress/wordpress-develop/pull/4949

This PR moves the hook that updates the wp_navigation post response schema to a public method of WP_Navigation_Fallback and deletes the single file.

Why? To remove the necessity to add a single file for the hook.

[kirasong]

Replying to isabel_brison:

<snip>
I don't believe so! It should be fine to move to an existing file.

Probably best aim for 6.3.1 at this point :)

As there isn't consensus here still, barring additional information, I'm leaning towards not changing this right before freeze as well (not meant as a blocking decision either way -- meant as feedback).

Because it seems like some of the original decision + consensus was with REST API folks (and I'm having trouble finding exactly who), pinging the component maintainers, in case they have any feedback here:
cc: @TimothyBlynJacobs @rmccue @rachelbaker @kadamwhite @spacedmonkey

[TimothyBlynJacobs]

This would be great to improve. But I agree, it doesn't need to block release and can be done in 6.3.1.

[kirasong]

Because it seems like the consensus here is to move this to 6.3.1, I'm going to go ahead and do that.

Then, depending on the solution, we can decide whether it's appropriate to ship in a minor release or not.

Thanks so much everyone for your work on this!

[audrasjb]

WP 6.3.1 is going to be released in the next few days, so let's move this ticket to 6.3.2 to give it more time to be committed and backported.

[joemcgill]

With us prepping for 6.3.2 and the fact that this hasn't seen any movement in a while, I'm going to push this to the 6.4 milestone. If someone can own committing this to trunk prior to that release, it could be moved back.

[oglekler]

This ticket was discussed during bug scrub.

We are getting closer to RC1 and due to this I am rescheduling this ticket to the next milestone.

Add props: @mukesh27

[SergeyBiryukov]

Replying to ramonopoly:

If we don't want to extend WP_REST_Posts_Controller, what about adding the hook as a public method of WP_Navigation_Fallback?

Indeed, ​PR 4957 appears to be the most straightforward way to resolve this for now.

​PR 4949 could be an option too, though if my reading of comment:16 is correct, whether or not extending WP_REST_Posts_Controller is the correct approach here could use some more discussion.

I think we can proceed with ​PR 4957 at this time, and perhaps explore the other option in a future release.

[SergeyBiryukov]

In 56793:

Editor: Move wp_navigation schema updating to WP_Navigation_Fallback class.

This aims to better align the navigation fallback implementation with core architecture and best practices.

The function that updates the wp_navigation post response schema is now a public method of the WP_Navigation_Fallback class, so an extra file previously used for that specific function is no longer necessary.

Follow-up to [56052].

Props ramonopoly, scruffian, isabel_brison, mukesh27, swissspidy, rajinsharwar, afercia, audrasjb, mikeschroder, JeffPaul, johnjamesjacoby, TimothyBlynJacobs, oglekler, SergeyBiryukov.
Fixes #58910.

Thanks for the PR! Merged in r56793.

Thanks for the PR! An alternative approach was merged in r56793, and this could be explored in a future release.