Show all template parts using the edited navigation #55782
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,3 +57,121 @@ | |
| ); | ||
| } | ||
| add_action( 'admin_menu', 'gutenberg_menu', 9 ); | ||
|
|
||
| if ( ! function_exists( 'html_contains_block' ) ) { | ||
| /** | ||
| * Returns whether the given HTML contains a block | ||
| * of the given type and, if provided, | ||
| * a given attribute and attribute value. | ||
| * | ||
| * Note that it's not possible to search for an attribute | ||
| * whose value is `null`. | ||
| * | ||
| * @param string $html The html to search in. | ||
| * @param string $block_name Find this block type, | ||
| * with an optional "core/" namespace, | ||
| * e.g. "paragraph", "core/paragraph", | ||
| * "my_plugin/my_block". | ||
| * @param string $attribute_name If provided, the block must also | ||
| * contain this attribute. | ||
| * @param string $attribute_value If provided, the given attribute's | ||
| * value must also match this. | ||
| * | ||
| * @return bool True if block is found, false otherwise | ||
| */ | ||
| function html_contains_block( $html, $block_name, $attribute_name = null, $attribute_value = null ) { | ||
|
There was a problem hiding this comment. I forgot to include in previous review but I think it would be wise to have some tests for this function. You could use a PHPUnit data provider to pass in various combinations of values to ensure you've covered a range of scenarios. One such example would be what @dmsnell said about Latin characters. |
||
| $at = 0; | ||
|
|
||
| /** | ||
| * This is the same regex as the one used in the block parser. | ||
| * It is better to use this solution to look for a block's existence | ||
| * in a document compared to having to parsing the blocks in the | ||
| * document, avoiding all the performance drawbacks of achieving | ||
| * a full representation of block content just to check if one block | ||
| * is there. | ||
draganescu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * | ||
| * @see WP_Block_Parser. | ||
| */ | ||
| $pattern = sprintf( | ||
| '~<!--\s+?wp:%s\s+(?P<attrs>{(?:(?:[^}]+|}+(?=})|(?!}\s+/?-->).)*+)?}\s+)?/?-->~s', | ||
| preg_quote( str_replace( 'core/', '', $block_name ), '~' ) | ||
| ); | ||
|
There was a problem hiding this comment. you probably already know this, but we need to expand this a bit because then we'd overlook posts containing given the fact that we're only looking for a single block type I recommend we hard-code the regex pattern to match the sought block name. we can do that by breaking apart the block name into its parts, different than the code I shared above about joining them. // check my string-index math
$solidus_at = strpos( $block_name, '/' );
$sought_namespace = false === $solidus_at ? 'core' : substr( $block_name, 0, $solidus_at );
$sought_name = false === $solidus_at ? $block_name : substr( $block_name, $solidus_at + 1 );
$pattern = sprintf(
'~<!--\s+?wp:((?:%s/)?%s\s+...~',
preg_quote( $sought_namespace, '~' ),
preg_quote( $sought_name, '~' )
); |
||
|
|
||
| while ( 1 === preg_match( $pattern, $html, $matches, PREG_OFFSET_CAPTURE, $at ) ) { | ||
| $at = $matches[0][1] + strlen( $matches[0][0] ); | ||
|
|
||
| if ( ! isset( $attribute_name ) ) { | ||
| return true; | ||
| } | ||
|
|
||
| $attrs = json_decode( $matches['attrs'][0], /* as-associative */ true ); | ||
| if ( ! isset( $attrs[ $attribute_name ] ) ) { | ||
draganescu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| continue; | ||
| } | ||
|
|
||
| if ( ! isset( $attribute_value ) ) { | ||
| return true; | ||
| } | ||
|
|
||
| if ( $attribute_value === $attrs[ $attribute_name ] ) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
| } | ||
| if ( ! function_exists( 'get_template_parts_that_use_menu' ) ) { | ||
| /** | ||
| * Get all template parts that use a menu. | ||
| * | ||
| * @param int $wp_navigation_id The menu id. | ||
| * | ||
| * @return array template parts that use the menu | ||
| */ | ||
| function get_template_parts_that_use_menu( $wp_navigation_id ) { | ||
|
|
||
| $wp_template_part_posts = get_posts( | ||
| array( | ||
| 'post_type' => 'wp_template_part', | ||
| 'posts_per_page' => -1, | ||
|
There was a problem hiding this comment. it looks like we abandoned all limits here, but this is being added to the query for template parts. there could still be a kind of time-bomb in here because some sites will have hundreds if not thousands of template parts, and we're not able to limit our search. this means that for every request to fetch template parts, we will be processing every template part in the database. it'd be great if we could impose an arbitrary limit or only add this if a specific query arg requests it so that we don't accidentally make the trivial case obstructively slow, when someone doesn't need to know what template parts use the given menu. if we limit to an arbitrary amount it would seem like we could add some kind of next-page link or cursor id to continue the search on-demand. There was a problem hiding this comment. @dmsnell I thought this was only called to get the field value when the REST query is for How is it being added to the query for template parts? Maybe I missed something? If I'm correct, then we could still further optimise this approach by only including the field if certain query params are provided. We don't need it for standard requests. I don't know the best REST etiquette for that but some REST maintainers might be able to assist. There was a problem hiding this comment.
yeah that was my mistake. still, my point is that we're time-bombing the API and I can imagine a handful of normal cases where all we want is the list of this could go really bad on real sites with lots of template parts. say we have a site with a thousand template parts and one navigation menu. even with that single nav menu, we still have to scan all thousand template part posts every time we request the list of nav menus. There was a problem hiding this comment. Let's set a hard limit on the number of items returned in the query. However we want to avoid situations where there are a lot of template parts which causes us to miss the key template parts for the site. To avoid this we could also consider optimising for common template parts such as There was a problem hiding this comment.
doing what we're trying to do here I think is going to require a search index, and one we'll likely need to build and maintain ourselves. I just don't see it being practical to scan the database every time we want to grab that can be done, but it's probably easier to do on its own.
if we have and can get a There was a problem hiding this comment.
I agree which is why I think this comes back around to limiting the scope of this change to it's own endpoint for the block editor only. This way we do not register this field on the Posts Controller responses at all. We can easily introduce a new private Core Data selector which queries this endpoint only when necessary. This will avoid hitting the DB each time there is a query for If we want to apply further safety limits we can do that as well. |
||
| ) | ||
| ); | ||
|
Comment on lines
+134
to
+139
There was a problem hiding this comment. I think we'll want to use |
||
|
|
||
| $wp_template_part_posts_with_navigation = array(); | ||
| foreach ( $wp_template_part_posts as $wp_template_part_post ) { | ||
| $found_navigation = html_contains_block( | ||
| $wp_template_part_post->post_content, | ||
| 'navigation', | ||
| 'ref', | ||
| $wp_navigation_id | ||
| ); | ||
| if ( $found_navigation ) { | ||
| $wp_template_part_posts_with_navigation[] = $wp_template_part_post->ID; | ||
| } | ||
| } | ||
| return $wp_template_part_posts_with_navigation; | ||
| } | ||
| } | ||
|
|
||
| if ( ! function_exists( 'register_template_parts_that_use_menu_field' ) ) { | ||
| /** | ||
| * Register a rest field for posts that returns the template parts that use the menu. | ||
| */ | ||
| function register_template_parts_that_use_menu_field() { | ||
| register_rest_field( | ||
| 'wp_navigation', | ||
| 'template_parts_that_use_menu', | ||
| array( | ||
| 'get_callback' => function ( $post ) { | ||
| return get_template_parts_that_use_menu( $post['id'] ); | ||
| }, | ||
| 'schema' => array( | ||
| 'type' => 'array', | ||
| 'context' => array( 'edit' ), | ||
| ), | ||
| ) | ||
| ); | ||
| } | ||
| } | ||
| add_action( 'rest_api_init', 'register_template_parts_that_use_menu_field' ); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: we'll want to add
?string(I'm pretty sure vs.string|null) before merging