Show all template parts using the edited navigation #55782
Conversation
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/init.php |
|
Flaky tests detected in c9ddbfa. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7607871618
|
draganescu
added
[Type] Enhancement
This |
|
Size Change: +2.04 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
draganescu
force-pushed
the
try/navigation-relation-meta
branch
from
23d8ced
to
f63e73d
Compare
|
I am thinking a better approach here is to extend the search endpoint to support a has_blocks param for post search with optional filtering by attribute name/value. This has_blocks param should not augment WP_Query but instead be treated separately as:
This is a very expensive operation, the default limits should be low. On the other hand it's perfectly cache-able. This would allow the generalisation of the feature here to apply to multiple scenarios:
|
|
Great exploration. Perhaps it would be useful to bring this up in REST API channels and discuss options with them if you haven't already? |
|
I took a week pause from this work due to other work, will resume now. |
draganescu
force-pushed
the
try/navigation-relation-meta
branch
from
0a681c9
to
c008f80
Compare
|
I've tested the idea of a controller that searches all posts based on
The main blocker is that it's hard to optimize this sort of search. That controller pages through the DB until it has found enough matching posts. If we want all by default it's not feasible. I think the other option - also in this PR - with just registering extra rest response fields that expose these relationships between posts based on blocks (template_part using wp_navigation with ref of certain value) is best. I will continue on this path. I need to see what place is best for registering / adding the extra field. |
There was a problem hiding this comment.
There's quite a timebomb locked inside of this logic. Someone is going to come along and want to add a cache as well. It would be good to provide some guidance on how that could go well or go wrong.
I've suggested a "bandaid" which addresses the inner loop without addressing the broader issue, of needing to scan through a site's posts.
At a minimum it seems like there should be some hard constraints built into this: a time limit imposed when searching, possibly the max number of posts to scan.
If we're relying on this in the editor I could imagine it being a really hard pause when loading for a site with thousands or tens of thousands of posts. We have no guarantee that posts contained the searched-for block exist in the "front" of the query either; it could be 3000 posts and then the first one with our block.
Mitigating this might imply something even more inherent, such as ensuring that the editor expects a progressively loading and asynchronous response. That is, setup an API call and show a spinner and start showing matching posts as they come in, but have the API call poll until it's done. This might involve creating a cursor to associate the search.
|
As an idea of an interface that recognizes that this could take tens of seconds or minutes or longer to response, we could create an actual search resource which can be later polled for streaming results. first matching posts… still thinking about it, scanning through posts but not finding any matches… all posts have been scanned and there is nothing left to scan… This gives the client the information it needs to start showing a UI, to grab the next batch of matched posts, to know when to continue polling, and when to stop. There's a problem here in that we don't have guarantees about how long it will take to find, say, the next five posts, so an API response might come at some time-based interval (say, give up and return after 3 seconds) but which should still indicate that the client needs to continue to poll. Obviously there are concurrency issues with posts being updated in the background, plus the fact that we want to avoid making this query multiple times from the client, but we also need to recognize that with time, the same query might produce different inherent results. |
|
I know fully well the time bomb that is why I am moving slowly here :). Thanks for all the super valuable consideration. |
|
@dmsnell I wonder if the original approach of the PR which aims only to show which template part uses a navigation block, and does that only to scan through template parts, and adds a rest field to the Then I can start a PR for a more generic locator resource that implements the optimisations and documentation you mentioned above. |
if it didn't involve creating a new public resource/endpoint/value I would think that's a fair proposal, but there's a risk here in that this only really kicks the can down the road, proverbially. a site with hundreds of template parts will exhibit the same problem and people will say "Gutenberg is not suitable for large agency work." I recently explored using chunked responses to stream the reply of a long-running API call, but I'm pretty sure that's unworkable with WordPress' "REST" facilities, and it would probably end up timing out at the kinds of delays I would expect this endpoint to return. creating the generic resource locator may not be as hard as it seems. we might be able to build it with a transient and it only needs to store the offset and query parameters to generate the next query batch/page. that transient can disappear after five minutes and everything will be fine; clients should expect that these cursors might fail or disappear. |
|
Thank you for working on this @draganescu Andrei! It is very helpful to be able to click into a Navigation to get information on where it is being used. Next up. |
|
@paaljoachim that's a good suggestion, but not for this PR, it'd be great to make an issue with a mockup to illustrate your idea which I like quite a lot.
I think register a field, in the site editor or by the navigation block itself, it does not expose anything other way to access this underperformat-by-design feature. I am not too afraid of hundreds of template parts. I could even bail at 50, sorted desc by most recent (b/c you want to see this info in current parts) and filter this number so people who trust their servers can bump it. I will proceed to remove the public resource and register the rest field in the response for |
|
An issue for brainstorming how to switch menu locations in the Navigation sidebar: |
This might mean settling for responses with zero results, because we can't guarantee that we will find fifty without scanning through hundreds or thousands of posts before we get the first fifty containing the block. |
No, I meant scan only 50. And yes, emtpy results for this particular problem it's fine. |
draganescu
force-pushed
the
try/navigation-relation-meta
branch
2 times, most recently
from
8972455
to
d4482f3
Compare
…te parts that use the current wp_navigation
draganescu
force-pushed
the
try/navigation-relation-meta
branch
from
d4482f3
to
bf8dcdb
Compare
| $wp_template_part_posts = get_posts( | ||
| array( | ||
| 'post_type' => 'wp_template_part', | ||
| 'posts_per_page' => -1, | ||
| ) | ||
| ); |
There was a problem hiding this comment.
I think we'll want to use WP_Query here with the various parameters that optimise the query performance such as no_found_rows...etc.
lib/init.php
Outdated
|
|
||
| if ( ! function_exists( 'html_contains_block' ) ) { | ||
| /** | ||
| * Recursively find a block by attribute. |
There was a problem hiding this comment.
Algorithmically this is no longer recursive, nor is it conceptually. I'm not sure this is helpful to mention "recursive" because it might lead someone astray trying to figure out how it's recursive.
We might say something more to the point about the goal:
Returns whether the given HTML contains a block of the given type
and, if provided, a given attribute and attribute value.
There was a problem hiding this comment.
Ah copy pasta sorry!
lib/init.php
Outdated
| // Don't parse JSON if it's not possible for the attribute to exist. | ||
| if ( ! str_contains( $matches['attrs'][0], "\"{$attribute_name}\":" ) ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
interesting note, but I believe this check can fail if the key is using non-latin1 characters. I wonder if it would actually be better to omit this optimization (which again, sorry for recommending it and then recommending its removal) for now, or note the discrepancy as a clue for someone in the future. we have to avoid situations like {"\u0061ge": 42} failing to match for age
| $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 wp_navigation posts? The get_callback invokes this function to get the field value for the template_parts_that_use_menu field.
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.
when the REST query is for wp_navigation posts?
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 wp_navigation posts. making the field opt-in would remove that burden and only charge the cost of computation when it's wanted.
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 header and footer to ensure these are always checked first. It's likely to serve the 80% use case.
There was a problem hiding this comment.
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.
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 wp_navigation items.
that can be done, but it's probably easier to do on its own.
serve the 80% use case
if we have and can get a header and footer quickly then that would seem like a good idea
There was a problem hiding this comment.
to scan the database every time we want to grab wp_navigation items.
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 wp_navigation to Posts Controller.
If we want to apply further safety limits we can do that as well.
| $wp_template_part_posts = get_posts( | ||
| array( | ||
| 'post_type' => 'wp_template_part', | ||
| 'posts_per_page' => -1, |
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 wp_navigation posts? The get_callback invokes this function to get the field value for the template_parts_that_use_menu field.
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.
| * | ||
| * @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.
|
Excellent, I am happy this is on track now. I will:
|
I have previously received feedback from REST maintainers that extending existing REST endpoints in arbritary ways for specific posts is not considered best practise. This is because (as you know) Gutenberg is not the only consumer of the API. You may find that a dedicated block editor specific endpoint (such as we implemented for Navigation Fallbacks) would be a better approach. Certainly this is what I was previously advised. |
|
Thanks for the useful info @getdave - I'll need to see how that applies to this particular situation. Either way I have no attachment to how more I have it to what which is to hide the html seek function for a while until we figure out the best tweaks to it. |
| * "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 |
There was a problem hiding this comment.
nitpick: we'll want to add ?string (I'm pretty sure vs. string|null) before merging
| $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 core/template, for example. although the editor prunes the core/ namespace, it's perfectly valid to have it written in the HTML.
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, '~' )
);
|
I am going to close this for now and come back to it with a better plan or this plan but a fresh implementation. |
Co-authored-by: Dennis Snell [email protected]
What
This PR shows which template parts use a navigation menu when the menu is edited in isolation in the site editor:
Screen recording
menu-used-in.mp4