#58910 closed defect (bug) (fixed)
Reconsider wp_add_fields_to_navigation_fallback_embedded_links() location
Reported by: | SergeyBiryukov | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 6.4 | Priority: | normal |
Severity: | normal | Version: | 6.3 |
Component: | Editor | Keywords: | has-patch has-unit-tests dev-feedback |
Focuses: | rest-api | Cc: |
Description
[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.
Change History (35)
This ticket was mentioned in PR #4916 on WordPress/wordpress-develop by @rajinsharwar.
17 months ago
#2
- Keywords has-patch added
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
#3
@
17 months ago
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.
#4
@
17 months ago
+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.
@audrasjb commented on PR #4916:
17 months ago
#5
I fixed the docblock issue in ce29cf0 FYI
#6
follow-up:
↓ 8
@
16 months ago
@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 :)
#7
@
16 months ago
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
#8
in reply to:
↑ 6
@
16 months ago
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.
#9
follow-up:
↓ 22
@
16 months ago
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 :)
#11
@
16 months ago
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.
#12
@
16 months ago
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 ticket was mentioned in PR #4949 on WordPress/wordpress-develop by @ramonopoly.
16 months ago
#13
- Keywords has-unit-tests added
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:
#14
@
16 months ago
- Keywords has-unit-tests removed
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:
- be only manipulating the
wp_navigation
post object returned by/navigation-fallback
embedded link response object or - 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.
@scruffian commented on PR #4949:
16 months ago
#16
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.
@scruffian commented on PR #4949:
16 months ago
#17
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.
#18
@
16 months ago
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 🤣
#19
@
16 months ago
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
#20
follow-up:
↓ 31
@
16 months ago
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
This ticket was mentioned in PR #4957 on WordPress/wordpress-develop by @ramonopoly.
16 months ago
#21
Alternative to:
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.
#22
in reply to:
↑ 9
@
16 months ago
- Keywords dev-feedback added
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
This ticket was mentioned in Slack in #core by priethor. View the logs.
16 months ago
#24
@
16 months ago
This would be great to improve. But I agree, it doesn't need to block release and can be done in 6.3.1.
#25
@
16 months ago
- Milestone changed from 6.3 to 6.3.1
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!
#26
@
16 months ago
- Milestone changed from 6.3.1 to 6.3.2
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.
This ticket was mentioned in Slack in #core by joemcgill. View the logs.
15 months ago
#28
@
15 months ago
- Milestone changed from 6.3.2 to 6.4
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.
This ticket was mentioned in Slack in #core by oglekler. View the logs.
14 months ago
#30
@
14 months ago
- Milestone changed from 6.4 to 6.5
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
#31
in reply to:
↑ 20
@
14 months ago
Replying to ramonopoly:
If we don't want to extend
WP_REST_Posts_Controller
, what about adding the hook as a public method ofWP_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.
#32
@
14 months ago
- Owner set to SergeyBiryukov
- Resolution set to fixed
- Status changed from new to closed
In 56793:
@SergeyBiryukov commented on PR #4957:
14 months ago
#34
Thanks for the PR! Merged in r56793.
@SergeyBiryukov commented on PR #4949:
14 months ago
#35
Thanks for the PR! An alternative approach was merged in r56793, and this could be explored in a future release.
Big +1. It really does not make sense as a standalone function in core.