Consolidate Navigation fallbacks logic between Editor and Front of Site via REST API #48698
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
|
Flaky tests detected in db39cdb. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4763249844
|
getdave
force-pushed
the
try/use-dedicated-rest-to-get-nav-fallbacks
branch
from
91388f2
to
56e702c
Compare
getdave
commented
Mar 22, 2023
getdave
commented
Mar 22, 2023
getdave
commented
Mar 22, 2023
getdave
commented
Mar 22, 2023
|
Size Change: -301 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
getdave
commented
Mar 23, 2023
getdave
commented
Mar 23, 2023
getdave
commented
Mar 23, 2023
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
I would love for them not to be public but we are deprecating a load of functions out of |
This comment was marked as resolved.
This comment was marked as resolved.
getdave
added
REST API Interaction
getdave
commented
Mar 24, 2023
getdave
commented
Mar 24, 2023
getdave
commented
Mar 24, 2023
getdave
commented
Mar 24, 2023
getdave
commented
Mar 24, 2023
getdave
commented
Mar 24, 2023
|
This PR has a great discussion on architectural design patterns, benefits of each for Core, and when/why to use procedural, OOP, or static class wrapper designs. Thank you @spacedmonkey for starting it. IMO this architectural design should not block this PR and should be a more broad, project-level discussion than just this one PR. Why? 👉 Next Steps Suggestion: I suggest the following:
What do you all think? Is this a good way forward? |
|
@hellofromtonya This seems like a pragmatic way of moving forward whilst acknowledging the various viewpoints expressed in this PR. I would happily commit to refactoring the code in this PR based on the outcome of any future discussion (as outlined by @hellofromtonya above). |
spacedmonkey
approved these changes
Apr 21, 2023
|
@hellofromtonya @azaozz Let's not drag this PR down with code design conversion. Let's move this conversion ( which is important ) elsewhere. |
|
Sorry, if it's not too much trouble can we change the REST API route from |
Yup no problem - I'll raise a follow up. Funny how we did all that renaming and no one noticed the name of the actual endpoint 🤦 |
This was referenced Apr 21, 2023
|
Thanks to everyone who contributed and/or reviewed this PR. It's great to see it merged. Also thanks to Core committers here for the pragmatic approach to moving the discussion around code architectural to a higher level and thus unblocking this PR. Our users will thank you for it! I've raised a series of follow ups in #49999. I will start addressing those soon. |
@TimothyBJacobs Follow up ready in #50044 |
bph
added
[Block] Navigation
This was referenced May 4, 2023
|
Note to documentation team: this PR changes behaviour so that in almost every scenario a Navigation block will have a menu assigned to it. It's now quite difficult to have an empty Navigation block. |
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?
Centralises all logic around finding / creating an appropriate fallback menu for the Navigation block onto the PHP side. Briefly this involves:
extending the Navigation REST endpoint with aadding a new block editor specific endpoint for retrieving the fallback./fallbacksendpoint.Why?
An empty Navigation block (one without a
refto an existing Nav Menu) should display a fallback Navigation Menu. The behaviour should be:The current approach in
trunkhas several flaws. At a high level these are:This PR aims to solve that by centralising all the logic around the creation/retrieval of fallback Navigation Menus onto the PHP side. This has the benefit of a single implementation of the logic which can be shared between client (via the REST API) and server (via direct access to the controlling class).
👀 Please also see
Common Questionsbelow.How?
Adds dedicated class to encapsulate all the domain knowledge and logic around the creation of Navigation Menu fallbacks.
Code previously in the Navigation block
index.phpis migrated to the class and all public functions are deprecatedand proxied to the appropriate class methods.Nav block
render_callbacknow calls the class to retrieve the fallback - this means code for the behaviour of "empty" Navigation blocks on the front of the site is using the central source of truth.A custom REST API endpoint for Navigation is created which exposes a new
fallbacksendpoint which can be called to create/retrieve a fallback Navigation Menu. This endpoint is accessed from the Navigation block code in the editor - this means code for the behaviour of "empty" Navigation blocks in the editor is using the central source of truth.This PR does not update client side code. However, if this PR lands, then it will be possible to drastically simplify the block implementation.It was decided this PR would make more sense if we included the changes to the editor code.Common Questions
Why not modify the existing
getendpoint (wp/navigation/{recordKey}) to create a menu if it doesn't exist?That endpoint is designed to retrieve a specific post by ID. It should not have a side effect of creating a post.
Moreover, an empty Navigation block is not going to request a post by ID. Rather it needs to know which post it should display. Therefore a dedicated endpoint seems logical.
Why not use filters to modify the response from the existing endpoint to create a menu if one doesn't exist
Same reason as above. Filtering the API to do a
CREATEaction when fetching from an endpoint which is part of an established WP REST API design pattern (e.g.Posts_Controller) would be an anti-pattern and provide poor DX.We should try to avoid modifying existing endpoints for something as custom as this.
Moreover, using this method would not allow the feature to be extended in future to accommodate new requirements such as the ability to find the most suitable fallback by template part slug (see below).
We should not extend the WP REST API just for the Gutenberg Plugin
This depends on whether you view Gutenberg as part of WP Core. The Gutenberg Plugin is a feature plugin, but much of its code ends up as part of Core. There are numerous examples of API endpoints starting in the Plugin ending up as Core REST endpoints (Theme JSON for example).
The Gutenberg Plugin should try to avoid modifying existing Core endpoints to conform to Gutenberg specific requirements. For example, a
GETendpoint forREADon a single post should not be extending to have additional side effects. However, it seems reasonable to allow the Plugin to extend existing controllers with additional endpoints as is done in this PR. This ensures we retain backwards compatibility, don't break existing REST design patterns whilst still facilitating the end goal.Ultimately the editor is part of Core so REST should open for modification to facilitate the optimal UX for the editor.
What other approaches have you tried/considered?
We have two options that have been explored:
trunkwhereby fallback logic is handled in the editor and PHP render separatelyNeither seems ideal. The former has several disadvantages including:
The later option is an improvement but has downsides including creating posts when the user may not need/want them (for example if they are happy with their existing Classic Menu).
As a result of these concerns, exploring the approach in this PR seemed like a good option.
What other benefits do you see in this new endpoint?
Currently the fallback mechanic is pretty primitive relying on date ordering to determine which Navigation Menu to show as a fallback. In the future however, it should be possible to pass a "hint" to this endpoint to provide it with guidance on selecting the most appropriate fallback.
The mechanics for "most appropriate" may change over time (imagine the possibiities for AI here) but in the short term I see Gutenberg utilising this by passing the
slugof the closest containing template part and the editor using that to determine which is the best menu to display.Say for example I pass the slug
twentytwentythree//header- the endpoint should have a basic heuristic to determine that the most suitable menu is likely to be one that has some properties that indicate it is a "header" menu. At a basic level this would be checking for Navigation Menus containing the wordheaderin thepost_titleor slug (post_name). The same is true for footer/sidebar .etc.Testing Instructions
Run the endpoint tests:
...and the domain class tests:
Manual testing
ref) in the Header of the site.Default fallbacks
Classic fallbacks
Most recent Navigation menu fallback
refattribute. You can do this by simply resetting the Template Part where you block resides.Multiple Navigation blocks
Testing Instructions for Keyboard
Screenshots or screencast
Followups
index.phpinto a separate file calleddeprecated.phpandrequirethat from the mainindex.php. This will reduce bloat in that file and discourage further modification.apiFetchusage in the Nav block editor code.linkto the the REST response for the fallbacks controller which indicates how to retrieve the fullwp_navigationPost object. Test that include the_embedon the request will embed ("inline") the full post object in the response.WP_Classic_To_Block_Menu_ConverterCloses #47058