Navigation: Refactor the PHP render function to make it easier to make changes in the future #55605

scruffian · 2023-10-25T10:00:59Z

What?

This refactors the PHP render function for the navigation block.

Why?

The aims are:

To make the code easier to grok.
To keep variables contained to the context in which they are used
To make it easier to change the code in the future.

How?

Creates a new class called WP_Navigation_Block so that the new functions can be kept private.

Testing Instructions

Check that the navigation block continues to behave as before.

Note

This removed the .is-fallback CSS class from the navigation block when it's in a fallback state. We don't use this class in core and I can't see a value to it for themes or users.

scruffian · 2023-10-26T08:13:25Z

packages/block-library/src/navigation/index.php

+	/**
+	 * Returns whether or not a navigation has a submenu.
+	 */
+	private static function does_navigation_have_submenus( $inner_blocks ) {


My main concern is that this function is now called several times so its probably slightly less performant.

We could exit the loop early and return from the method if a submenu is found. I think continue will do that 🤔

now called several times

Why is this different than before?

probably slightly less performant.

Also this is likely to be marginal. Is it a big concern?

~~Let's create an instance variable to cache the result of the function. Then we can check if the instance variable has already been assigned and exit early with that result.~~

Actually it's a static class so doing this makes no sense. The class doesn't not represent a block instance.

I'm now wondering if the class should not be static. The instance represents a Navigation and the class is a blue print for hanlding that Nav

Why is this different than before?

Before it was called in the render function and saved in a variable.

Also this is likely to be marginal. Is it a big concern?

No

getdave

Good PR 👍

Initial thoughts...

packages/block-library/src/navigation/index.php

getdave · 2023-10-26T10:08:35Z

packages/block-library/src/navigation/index.php

+	/**
+	 * Returns whether or not a navigation has a submenu.
+	 */
+	private static function does_navigation_have_submenus( $inner_blocks ) {


We could exit the loop early and return from the method if a submenu is found. I think continue will do that 🤔

getdave · 2023-10-26T10:09:12Z

packages/block-library/src/navigation/index.php

+	/**
+	 * Returns whether or not a navigation has a submenu.
+	 */
+	private static function does_navigation_have_submenus( $inner_blocks ) {


now called several times

Why is this different than before?

probably slightly less performant.

Also this is likely to be marginal. Is it a big concern?

getdave · 2023-10-26T10:14:03Z

packages/block-library/src/navigation/index.php

+	/**
+	 * Returns whether or not a navigation has a submenu.
+	 */
+	private static function does_navigation_have_submenus( $inner_blocks ) {


~~Let's create an instance variable to cache the result of the function. Then we can check if the instance variable has already been assigned and exit early with that result.~~

Actually it's a static class so doing this makes no sense. The class doesn't not represent a block instance.

I'm now wondering if the class should not be static. The instance represents a Navigation and the class is a blue print for hanlding that Nav

getdave · 2023-10-26T10:16:04Z

packages/block-library/src/navigation/index.php

-		$responsive_container_markup,
-		$nav_element_directives
-	);
+	return WP_Navigation_Block::render( $attributes, $content, $block );


So clean 🏅

getdave

More comments..

packages/block-library/src/navigation/index.php

getdave · 2023-10-26T10:17:51Z

packages/block-library/src/navigation/index.php

+	/**
+	 * Returns the markup for the navigation block.
+	 */


I'm looking at this thinking...

How is this different from render?

😕 Is there a better name?

packages/block-library/src/navigation/index.php

getdave

The thing I dislike about the PR is that we're refactoring without any test coverage. So the only way we will know if refactoring broke something is when a bug is found 😬

But given the lack of existing tests I don't see much option.

I think we should add unit tests for a select few methods at least,

github-actions · 2023-10-26T11:10:13Z

Flaky tests detected in 0e63a3a.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6785723162
📝 Reported issues:

[Flaky Test] should partially select with shift + click (@webkit) #53524 in /test/e2e/specs/editor/various/multi-block-selection.spec.js

…e changes in the future

Co-authored-by: Dave Smith <[email protected]>

MaggieCabrera

Besides the unit tests that we included on this PR, I've tested this comparing the markup generated for a navigation block that has:

a submenu
a site title block
a logo block
social icons

The markup generated on trunk and with this PR is exactly the same except for the is-fallback class that has been removed. I think this is good to go!

scruffian · 2023-11-09T12:14:59Z

I decided to merge this so that we don't end up with messy rebases when changes get made to the navigation render function. I expect there are still opportunities to improve this code though, so please leave more reviews and I'll follow up with more improvements. I'll also submit a core patch now so that we can get reviews from core committers.

scruffian · 2023-11-09T12:40:35Z

I opened a patch in core for this here: https://core.trac.wordpress.org/ticket/59867

Mamaduka · 2023-11-14T05:38:23Z

What's the strategy for patching bugs or making enhancements for this class after it ships in the core?

I'm asking because we started discouraging Gutenberg_ prefixed classes and functions in the block-library package, as they can easily slip in during package sync and break the core.

tellthemachines · 2023-11-14T06:52:48Z

What's the strategy for patching bugs or making enhancements for this class after it ships in the core?

What we've done previously with e.g. the theme json class is make a copy of the whole class in Gutenberg, namespaced so it doesn't conflict with the core one, and use it in the plugin instead of the core class. If we're using the class directly in the block library PHP files that makes things a little more complicated 😅 but I'm sure we can come up with some auto-renaming script that will switch the core class with the gutenberg one at build time.

…e changes in the future (#55605) * Navigation: Refactor the PHP render function to make it easier to make changes in the future * move code to get_inner_blocks_html * remove is-fallback class * allow multiple menus * add some comments * refactor directives to a different function * fix submenus * refactor responseive navigation function * remove a passed param * create a function for get_styles * dont pass the container_attributes as a param * handle view script loading * reorder code * simplify the return * tidy up the wrapper attributes * remove one dependence on has_submenu * remove the should load view script variable * more simplification * Update packages/block-library/src/navigation/index.php Co-authored-by: Dave Smith <[email protected]> * rename class * remname is_responsive_navigation * rename does_navigation_have_submenus * rename get_inner_blocks_for_navigation * rename get_layout_class_for_navigation * Update packages/block-library/src/navigation/index.php Co-authored-by: Dave Smith <[email protected]> * Update packages/block-library/src/navigation/index.php Co-authored-by: Dave Smith <[email protected]> * change name of get_nav_markup * split get_inner_blocks into smaller functions * add comment * remove unnecessary spaces * update docs * reverse parameter order * reverse parameter order * reverse parameter order * move the class to a new file * add docs * rename to * move function that's only used by the plugin into that section of the code * fix linter * reference the class internally using static * move variable to static variables * Update lib/compat/wordpress-6.5/class-wp-navigation-block-renderer.php Co-authored-by: Dave Smith <[email protected]> * Update lib/compat/wordpress-6.5/class-wp-navigation-block-renderer.php Co-authored-by: Dave Smith <[email protected]> * move the logic for inner block rendering to a separate function * test for navigation link markup * test for site title markup * removed comments, changed assertSame to assertEquals * format php * hide the condition in a function * hide the condition in a function * return early from has_submenus * comments for the tests * create a function to get the name * rename variable to nav_blocks_wrapped_in_list_item * phpcs * phpcs * Update phpunit/class-wp-navigation-block-renderer-test.php Co-authored-by: Dave Smith <[email protected]> * Update phpunit/class-wp-navigation-block-renderer-test.php * Update phpunit/class-wp-navigation-block-renderer-test.php * Apply suggestions from code review Co-authored-by: Dave Smith <[email protected]> * dynamic url for links on test --------- Co-authored-by: Dave Smith <[email protected]> Co-authored-by: MaggieCabrera <[email protected]>

tellthemachines · 2023-11-23T03:59:50Z

packages/block-library/src/navigation/index.php

+		}
+
+		$menu_items_by_parent_id = block_core_navigation_sort_menu_items_by_parent_id( $menu_items );
+		$parsed_blocks           = block_core_navigation_parse_blocks_from_menu_items( $menu_items_by_parent_id[0], $menu_items_by_parent_id );


I'm seeing a warning Deprecated: Function gutenberg_block_core_navigation_parse_blocks_from_menu_items is deprecated since version 6.3.0! Use WP_Navigation_Fallback::parse_blocks_from_menu_items instead. in /var/www/html/wp-includes/functions.php on line 6031 on trunk when using a hybrid theme with some legacy Navigation blocks in it. Would it be possible to use WP_Navigation_Fallback::parse_blocks_from_menu_items here?

getdave · 2024-01-25T10:54:57Z

phpunit/class-wp-navigation-block-renderer-test.php

+ * @package Gutenberg
+ */
+
+class WP_Navigation_Block_Renderer_Test extends WP_UnitTestCase {


@scruffian I think we should move this test into the phpunit/blocks directory. It is specific to a specific block and thus should be managed within the Gutenberg repo.

See https://github.com/WordPress/gutenberg/blob/trunk/docs/contributors/code/back-merging-to-wp-core.md#ignored-directoriesfiles

Done in #58247

getdave · 2024-01-25T10:56:23Z

Noting that the renderer file code was moved into the block-library package in #57979 and thus should be automatically handled by the packages sync process.

However, the phpunit test should be moved into phpunit/blocks to avoid the need to backport to WP Core.

oandregal · 2024-01-31T15:28:43Z

👋 We need a follow-up to address some performance issues. I've prepared #58506 to demonstrate.

scruffian added [Type] Code Quality Issues or PRs that relate to code quality [Block] Navigation Affects the Navigation Block labels Oct 25, 2023

scruffian requested review from draganescu, getdave, talldan and ajlende October 25, 2023 10:01

scruffian self-assigned this Oct 25, 2023

scruffian requested a review from tellthemachines as a code owner October 25, 2023 10:01

scruffian commented Oct 26, 2023

View reviewed changes

getdave reviewed Oct 26, 2023

View reviewed changes

getdave mentioned this pull request Oct 27, 2023

Fix broken list markup in navigation block when 3rd party blocks are used as decendants of navigation block #55551

Merged

scruffian added 15 commits November 2, 2023 14:34

Navigation: Refactor the PHP render function to make it easier to mak…

65f5793

…e changes in the future

move code to get_inner_blocks_html

5bcde61

remove is-fallback class

dcd97b7

allow multiple menus

e268651

add some comments

8f54d44

refactor directives to a different function

a9f0328

fix submenus

f087c4e

refactor responseive navigation function

b40caa5

remove a passed param

a6ffa85

create a function for get_styles

2991d1e

dont pass the container_attributes as a param

afe3474

handle view script loading

58906ca

reorder code

67d5ec3

simplify the return

40c4cce

tidy up the wrapper attributes

0544d33

MaggieCabrera and others added 5 commits November 8, 2023 09:36

Update phpunit/class-wp-navigation-block-renderer-test.php

163f08f

Co-authored-by: Dave Smith <[email protected]>

Update phpunit/class-wp-navigation-block-renderer-test.php

649f813

Update phpunit/class-wp-navigation-block-renderer-test.php

73764c9

Apply suggestions from code review

9b669a1

Co-authored-by: Dave Smith <[email protected]>

dynamic url for links on test

c56237d

MaggieCabrera approved these changes Nov 9, 2023

View reviewed changes

getdave requested a review from anton-vlasenko November 9, 2023 11:34

scruffian merged commit f3bb6d5 into trunk Nov 9, 2023
50 checks passed

scruffian deleted the refacotor/navigation-render branch November 9, 2023 12:13

github-actions bot added this to the Gutenberg 17.1 milestone Nov 9, 2023

scruffian mentioned this pull request Nov 22, 2023

Refactor Navigation block render function to be open for extension #52470

Closed

tellthemachines mentioned this pull request Nov 23, 2023

Call to undefined function block_core_navigation_get_menu_items_at_location #56453

Closed

tellthemachines reviewed Nov 23, 2023

View reviewed changes

tellthemachines added the Needs PHP backport Needs PHP backport to Core label Nov 23, 2023

talldan mentioned this pull request Nov 29, 2023

Fix error when using a navigation block that returns an empty fallback result #56629

Merged

getdave mentioned this pull request Jan 18, 2024

Plugin: Synchronize PHP changes for WordPress 6.5 release #57959

Closed

getdave reviewed Jan 25, 2024

View reviewed changes

scruffian mentioned this pull request Jan 25, 2024

Navigation: Move the Navigation block rendering tests to the blocks directory #58247

Merged

getdave removed the Needs PHP backport Needs PHP backport to Core label Jan 25, 2024

oandregal mentioned this pull request Jan 31, 2024

Navigation: minimize the number of innerblocks renders #58506

Closed

scruffian mentioned this pull request Jan 31, 2024

Navigation: Fix performance regression #58513

Merged

ockham mentioned this pull request Feb 8, 2024

Navigation block: Check Block Hooks API callback hasn't already been added. #58772

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Navigation: Refactor the PHP render function to make it easier to make changes in the future #55605

Navigation: Refactor the PHP render function to make it easier to make changes in the future #55605

scruffian commented Oct 25, 2023 •

edited

Loading

scruffian Oct 26, 2023

getdave Oct 26, 2023

getdave Oct 26, 2023

getdave Oct 26, 2023 •

edited

Loading

scruffian Oct 26, 2023 •

edited

Loading

getdave left a comment

getdave Oct 26, 2023

getdave Oct 26, 2023

getdave Oct 26, 2023 •

edited

Loading

getdave Oct 26, 2023

getdave left a comment

getdave Oct 26, 2023

getdave left a comment

github-actions bot commented Oct 26, 2023 •

edited

Loading

MaggieCabrera left a comment

scruffian commented Nov 9, 2023

scruffian commented Nov 9, 2023

Mamaduka commented Nov 14, 2023

tellthemachines commented Nov 14, 2023

tellthemachines Nov 23, 2023

getdave Jan 25, 2024

scruffian Jan 25, 2024

getdave commented Jan 25, 2024

oandregal commented Jan 31, 2024

Navigation: Refactor the PHP render function to make it easier to make changes in the future #55605

Navigation: Refactor the PHP render function to make it easier to make changes in the future #55605

Conversation

scruffian commented Oct 25, 2023 • edited Loading

What?

Why?

How?

Testing Instructions

Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

getdave Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

scruffian Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

getdave left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

getdave Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

getdave left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

getdave left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 26, 2023 • edited Loading

MaggieCabrera left a comment

Choose a reason for hiding this comment

scruffian commented Nov 9, 2023

scruffian commented Nov 9, 2023

Mamaduka commented Nov 14, 2023

tellthemachines commented Nov 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

getdave commented Jan 25, 2024

oandregal commented Jan 31, 2024

scruffian commented Oct 25, 2023 •

edited

Loading

getdave Oct 26, 2023 •

edited

Loading

scruffian Oct 26, 2023 •

edited

Loading

getdave Oct 26, 2023 •

edited

Loading

github-actions bot commented Oct 26, 2023 •

edited

Loading