Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Navigation Block: Prevent circular references in navigation block rendering #46387

Merged
merged 7 commits into from
Dec 8, 2022
Merged

Navigation Block: Prevent circular references in navigation block rendering #46387

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2aeff10
Revert "Handle innerContent too when removing innerBlocks (#46377)"
Dec 7, 2022
0569330
Revert "Add social link singular to list of blocks to be allowed (#46…
Dec 7, 2022
658ed3c
Revert "Recursively remove Navigation block’s from appearing inside N…
Dec 7, 2022
eb02095
Refactor ul code for readability
Dec 7, 2022
208e7ce
Refactor li wrapper conditional
Dec 7, 2022
cc64fd6
Prevent rendering nav refs that have already been seen
Dec 8, 2022
96e64e8
Fix PHP lint
Dec 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/block-library/src/navigation/edit/inner-blocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import { useMemo } from '@wordpress/element';
*/
import PlaceholderPreview from './placeholder/placeholder-preview';

// This list is duplicated in packages/block-library/src/navigation/index.php
const ALLOWED_BLOCKS = [
'core/navigation-link',
'core/search',
Expand Down
89 changes: 42 additions & 47 deletions packages/block-library/src/navigation/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,49 +390,20 @@ function block_core_navigation_get_most_recently_published_navigation() {
}

/**
* Recursively filter out blocks from the block list that are not whitelisted.
* This list of exclusions includes:
* - The Navigation block itself (results in recursion).
* - empty "null" blocks from the block list.
* - other blocks that are not yet handled.
*
* Note: 'parse_blocks' includes a null block with '\n\n' as the content when
* Filter out empty "null" blocks from the block list.
* 'parse_blocks' includes a null block with '\n\n' as the content when
* it encounters whitespace. This is not a bug but rather how the parser
* is designed.
*
* @param array $parsed_blocks the parsed blocks to be filtered.
* @return array the filtered parsed blocks.
* @param array $parsed_blocks the parsed blocks to be normalized.
* @return array the normalized parsed blocks.
*/
function block_core_navigation_filter_out_invalid_blocks( $parsed_blocks ) {
// This list is duplicated in /packages/block-library/src/navigation/edit/inner-blocks.js.
$allowed_blocks = array(
'core/navigation-link',
'core/search',
'core/social-links',
'core/social-link',
'core/page-list',
'core/spacer',
'core/home-link',
'core/site-title',
'core/site-logo',
'core/navigation-submenu',
);

$filtered = array_reduce(
function block_core_navigation_filter_out_empty_blocks( $parsed_blocks ) {
ajlende marked this conversation as resolved.
Show resolved Hide resolved
$filtered = array_filter(
$parsed_blocks,
function( $carry, $block ) use ( $allowed_blocks ) {
if ( isset( $block['blockName'] ) && in_array( $block['blockName'], $allowed_blocks, true ) ) {
if ( $block['innerBlocks'] ) {
$block['innerBlocks'] = block_core_navigation_filter_out_invalid_blocks( $block['innerBlocks'] );
if ( empty( $block['innerBlocks'] ) ) {
$block['innerContent'] = array( implode( $block['innerContent'] ) );
}
}
$carry[] = $block;
}
return $carry;
},
array()
function( $block ) {
return isset( $block['blockName'] );
}
);

// Reset keys.
Expand Down Expand Up @@ -472,8 +443,7 @@ function block_core_navigation_get_fallback_blocks() {

// Use the first non-empty Navigation as fallback if available.
if ( $navigation_post ) {

$maybe_fallback = block_core_navigation_filter_out_invalid_blocks( parse_blocks( $navigation_post->post_content ) );
$maybe_fallback = block_core_navigation_filter_out_empty_blocks( parse_blocks( $navigation_post->post_content ) );

// Normalizing blocks may result in an empty array of blocks if they were all `null` blocks.
// In this case default to the (Page List) fallback.
Expand Down Expand Up @@ -541,6 +511,7 @@ function block_core_navigation_from_block_get_post_ids( $block ) {
function render_block_core_navigation( $attributes, $content, $block ) {

static $seen_menu_names = array();
static $seen_ref = array();

// Flag used to indicate whether the rendered output is considered to be
// a fallback (i.e. the block has no menu associated with it).
Expand Down Expand Up @@ -611,6 +582,11 @@ function render_block_core_navigation( $attributes, $content, $block ) {

// Load inner blocks from the navigation post.
if ( array_key_exists( 'ref', $attributes ) ) {
if ( in_array( $attributes['ref'], $seen_ref, true ) ) {
return '';
}
$seen_ref[] = $attributes['ref'];
Comment on lines +585 to +588
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be smarter and keep track of the parent rather than a flat array so you could have two identical sibling navigation blocks, but Navigation blocks probably shouldn't be nested inside other navigation blocks in the first place, so it's probably fine this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ended up not being fine. #47085


$navigation_post = get_post( $attributes['ref'] );
if ( ! isset( $navigation_post ) ) {
return '';
Expand All @@ -631,7 +607,7 @@ function render_block_core_navigation( $attributes, $content, $block ) {

// 'parse_blocks' includes a null block with '\n\n' as the content when
// it encounters whitespace. This code strips it.
$compacted_blocks = block_core_navigation_filter_out_invalid_blocks( $parsed_blocks );
$compacted_blocks = block_core_navigation_filter_out_empty_blocks( $parsed_blocks );

// TODO - this uses the full navigation block attributes for the
// context which could be refined.
Expand Down Expand Up @@ -703,22 +679,41 @@ function render_block_core_navigation( $attributes, $content, $block ) {
_prime_post_caches( $post_ids, false, false );
}

$list_item_nav_blocks = array(
'core/navigation-link',
'core/home-link',
'core/site-title',
'core/site-logo',
'core/navigation-submenu',
);

$needs_list_item_wrapper = array(
'core/site-title',
'core/site-logo',
);

$inner_blocks_html = '';
$is_list_open = false;
foreach ( $inner_blocks as $inner_block ) {
if ( ( 'core/navigation-link' === $inner_block->name || 'core/home-link' === $inner_block->name || 'core/site-title' === $inner_block->name || 'core/site-logo' === $inner_block->name || 'core/navigation-submenu' === $inner_block->name ) && ! $is_list_open ) {
$is_list_item = in_array( $inner_block->name, $list_item_nav_blocks, true );

if ( $is_list_item && ! $is_list_open ) {
$is_list_open = true;
$inner_blocks_html .= '<ul class="wp-block-navigation__container">';
}
if ( 'core/navigation-link' !== $inner_block->name && 'core/home-link' !== $inner_block->name && 'core/site-title' !== $inner_block->name && 'core/site-logo' !== $inner_block->name && 'core/navigation-submenu' !== $inner_block->name && $is_list_open ) {

if ( ! $is_list_item && $is_list_open ) {
$is_list_open = false;
$inner_blocks_html .= '</ul>';
}

$inner_block_content = $inner_block->render();
if ( 'core/site-title' === $inner_block->name || ( 'core/site-logo' === $inner_block->name && $inner_block_content ) ) {
$inner_blocks_html .= '<li class="wp-block-navigation-item">' . $inner_block_content . '</li>';
} else {
$inner_blocks_html .= $inner_block_content;
if ( ! empty( $inner_block_content ) ) {
if ( in_array( $inner_block->name, $needs_list_item_wrapper, true ) ) {
$inner_blocks_html .= '<li class="wp-block-navigation-item">' . $inner_block_content . '</li>';
} else {
$inner_blocks_html .= $inner_block_content;
}
Comment on lines +682 to +716
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of this is refactoring as I was trying to make sense of the render function.

}
}

Expand Down