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.

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

Better block theme compatibility #728

Closed
boonebgorges opened this issue Oct 23, 2023 · 23 comments
Closed

Better block theme compatibility #728

boonebgorges opened this issue Oct 23, 2023 · 23 comments
Milestone

Comments

@boonebgorges
Copy link
Owner

On a default installation of WP running twentytwentytwo or twentytwentythree, the main post type directory puts the Docs-specific filters in a weird place. See screenshot:

Screenshot_2023-10-23_15-45-14

I don't know how to work around this, but let's consider this ticket a placeholder for figuring it out.

@boonebgorges
Copy link
Owner Author

I've spent a few minutes playing with this. The default WP block themes use wp:post-excerpt blocks on their archive templates. I played with several techniques for intercepting the way that this block is rendered in certain cases, but they were all so awful that I had to move on. Ultimately, the root problem is that need a wp:post-content block on the bp_doc archive page.

What feels like a "nice" solution is for Docs to provide its own template, perhaps one that it would recommend for installation when you install Docs or when you activate a block theme. But this poses its own problems, since we don't know what the rest of the template should look like - what the shared header/footer elements are in a given theme, etc.

So I came up with the following technique, which is ugly, but less ugly than other options. It does the following:

  1. Intervenes in the block template query (something like the "template stack" in the old BP theme compat layer) and identifies whether we're looking for an archive-bp_doc template. This ensures that we are targeting our fix
  2. Does some sanity checks to ensure that the user hasn't already provided a compatible template
  3. If no appropriate template is found, it makes a copy of the template that WP was planning to use, and simply swaps out post-excerpt for post-content

Again, this is not terribly elegant, but it seems to work, and it seems not to have negative effects for other archives etc.

Here's the magic:

/**
 * Provides a stub block template for the Docs directory.
 */
function bp_docs_provide_block_template_for_docs_directory( $templates, $query ) {
	// We are only concerned with the Docs directory, ie the bp_doc archive.
	if ( empty( $query['slug__in'] ) || ! in_array( 'archive-bp_doc', $query['slug__in'], true ) ) {
		return $templates;
	}

	// If an archive-bp_doc template was found, use it.
	$has_archive_bp_doc_template = false;
	foreach ( $templates as $template ) {
		if ( 'archive-bp_doc' === $template->slug ) {
			$has_archive_bp_doc_template = true;
			return $templates;
		}
	}

	// If the top template already has a wp:post-content block, use it.
	$first_template = reset( $templates );
	if ( false !== strpos( $first_template->content, 'wp:post-content' ) ) {
		return $templates;
	}

	// Copy the first template and swap out the wp:post-excerpt block for wp:post-content.
	$new_template          = clone $first_template;
	$new_template->slug    = 'archive-bp_doc';
	$new_template->title   = __( 'Docs Directory', 'buddypress-docs' );
	$new_template->content = str_replace( 'wp:post-excerpt', 'wp:post-content', $new_template->content );

	// Add the new template to the top of the list.
	array_unshift( $templates, $new_template );

	return $templates;
}
add_filter( 'get_block_templates', 'bp_docs_provide_block_template_for_docs_directory', 10, 2 );

@dcavins Since you've done some testing related to #738, perhaps you could run this across a couple different theme configurations to see whether you can break it.

@dcavins
Copy link
Collaborator

dcavins commented Dec 15, 2023

@imath had a good suggestion for how to handle this:
Using one of the template hierarchy filters, tell WP to use page.php instead of archive.php when we're looking at the BP Docs template.
Then, use the typical BP content replacement strategy that works on our single and edit screens, even in block land.

I spent some time going down this road, and it seems promising, but the template hierarchy/filters in block themes are really different, and I was having trouble finding the right combination to use. I learned some odd things, though, like, with 2022, the parameter being passed into the archive_template filter is /wp-includes/template-canvas.php and running locate_template( array( "page.php", $templates ) yields an empty string.

I will continue to pursue this, also. You can see my first try in https://github.com/dcavins/buddypress-docs/tree/wider-layout-support

@boonebgorges
Copy link
Owner Author

Hm. When you say page.php and archive.php do you mean page.html and archive.html? Because locate_template( [ 'page.php' ] ) generally fails in block themes, since block themes don't in fact have a page.php.

On a lark, I forced WP to use page.html for the archive page, but sadly this does not work either. That's because buddypress-docs leverages the query loop - ie the main WP_Query instance that is part of the WP bootstrap - for the archive pages. archive.html templates support this via the Query block: https://github.com/WordPress/twentytwentyfour/blob/df92472089ede6fae5924c124a93c843b84e8cbd/templates/archive.html#L8, https://github.com/WordPress/twentytwentyfour/blob/df92472089ede6fae5924c124a93c843b84e8cbd/patterns/posts-3-col.php#L10 The page.html template, on the other hand, doesn't have a Query block wrapper: https://github.com/WordPress/twentytwentyfour/blob/df92472089ede6fae5924c124a93c843b84e8cbd/templates/page.html So you end up with the proper template (including comments section) but a blank page.

I'm wary of a solution - php or html - that overrides more of the template than is necessary. If we force WP to use page.php and it falls back on the default template that ships with WP, we might end up with dramatic deviations in header/footer/style/markup from other pages on the site. Similarly, if we provide our own archive-bp_doc.html template, we won't share the structure of the parent theme. My block swap-out technique avoids these issues by leveraging the very same template that WP was already going to use, but forcing it to use the full-content block rather than the excerpt.

That being said, I could be looking in the wrong place, so I'm happy to be shown how the other approach could work :)

@imath
Copy link

imath commented Dec 16, 2023

Hi @boonebgorges & @dcavins This is an interesting ticket 😄 . I like @boonebgorges approach about replacing the serialized block.

FWIW when starting working on a Full Block Theme for BP, I've found that the Block Template hierarchy is "copying" the regular Template hierarchy simply replacing the .php extension with an .html one. I remember I was hesitant on using the archive template to display BP Loops, then I found it was too problematic to use this archive.html template, reason I suggested to @dcavins to use the page one.

boonebgorges added a commit that referenced this issue Dec 16, 2023
…lates.

Our technique involves providing a "fake" template that copies the
archive template and swaps out the excerpt block for a content block.

See #728.
@boonebgorges
Copy link
Owner Author

boonebgorges commented Dec 16, 2023

@imath Thanks for your thoughts. I've pushed up 6097e82, which contains my fix with some improved documentation. I've included it in the branch for #738 because the problems are ideally fixed together.

Thinking more about this, I think that the long-term "proper" fix for a plugin like buddypress-docs is to provide a real archive-bp_doc.html template. It would use the core query/post-template system to display its content, and be fully editable in the Site Editor. There are a couple of things that stand in the way of doing this today:

  1. We'd need a good number of custom blocks, which would take time to build. This includes the filters that appear at the top of the directory and some of the info (like "associated group") that appears in the table.
  2. This approach would create a divide between the way that the Docs directory looks when viewed at the CPT archive URL (/docs/) and within BP pages (/members/boone/docs/, /groups/boones-group/docs/). Those areas would continue to be powered by BP's theme compatibility layer, which is powered by PHP (BP's plugins.php, BP_Group_Extension, etc).
  3. We'd need a way to install or inject the template. I'm not sure whether the WP team has thought through this possibility, or if there's good tools in the API for a plugin to do so. The assumptions are generally that all "base" templates are provided by the theme, and you overwrite them with wp_template objects in the database - plugins aren't really involved in this flow.

I think that many of these questions will have to be answered by BuddyPress in one way or another, though in the case of BP it'll be much more complicated - BP has more content types, and they're not CPTs. But it might be worth thinking through the approach holistically.

Whatever the long-term approach, my proposed workaround will at least keep Docs functional on block themes in the near term.

@imath
Copy link

imath commented Dec 16, 2023

@boonebgorges I've been looking into Block themes for a year building one for my personal website and starting to experiment things in BuddyPress. In next major release I think, now we have BP Rewrites API in, we should make a first step towards being able to edit community area pages within the Site Editor.

As you wrote, we need to build more BP Blocks (header-item, body-item, primary-nav, secondary-nav, etc...) & probably an extended version of the WordPress pattern. As far as I can remember you can't provide context inside a pattern which is pretty annoying, the BP Block Pattern will need to provide a BP Context so that's easier to output the right content according to it.

Here's the steps we'll take:

  1. Include a small adaptation to BP Template loading: Use locate_block_template() to support BP Block only Themes buddypress/buddypress#104
  2. Work from the BuddyVibes standalone BP Block Theme on BP Blocks and BP Patterns
  3. Move the BP Blocks and BP Patterns into BuddyPress core

For now I chose to avoid the problem you perfectly described here:

The assumptions are generally that all "base" templates are provided by the theme, and you overwrite them with wp_template objects in the database - plugins aren't really involved in this flow.

@dcavins
Copy link
Collaborator

dcavins commented Dec 18, 2023

@boonebgorges Which block themes did you try with 6097e82?

At first blush, it seems to work in 2022 and 23, but not 24, so I'm curious which you've tried. I'm stunned that they don't all behave the same way! :D Thanks!

@boonebgorges
Copy link
Owner Author

I tested with twentytwentytwo and twentytwentythree. It's not working with twentytwentyfour because that theme loads its query template inside of a pattern rather than directly in the template file. So the string manipulation doesn't work. What a huge pain. I'll see if there's a way I can intercede after the patterns have been parsed.

boonebgorges added a commit that referenced this issue Dec 18, 2023
@boonebgorges
Copy link
Owner Author

boonebgorges commented Dec 18, 2023

Here's a case where we have to hack around a hack :)

My previous technique looked at the serialized content of the archive template to identify and then replace post-excerpts. This technique doesn't work with themes that don't have post-excerpt serialized directly in the template. twentytwentyfour, for example, uses a pattern, and the pattern in turn contains the post-excerpt block.

I've reworked my hack so that we descend the block tree and find the most "specific" block whose rendered HTML contains excerpt markup. In the case of twentytwentytwo and twentytwentythree, this will be the post-excerpt block itself, in which case we can use the previous technique. In the case of twentytwentyfour, it will be the pattern block. When that happens, we fetch the contents of the pattern from the pattern registry, and search that for a post-excerpt, swapping it out if found. Then, in our pseudo-template, we swap out the <wp:pattern> block in the template content with the contents of the updated serialized pattern. In other words, the fake bp_doc-archive.html template content no longer contains a wp:pattern block, but instead contains the blocks that belong to that pattern (with our swapouts).

This works, but:
a. It's crazy. We have to work like this because WP takes serialized templates, finds pattern blocks, uses PHP to get a PHP array of block objects, renders them recursively on the server, etc. At no point does WP ever have the whole post content in a single format that represents all the blocks, either serialized or in PHP. So there's no way for us to do a single check of all those blocks, and we have to do all this other weird stuff. It's worth noting that none of this would be necessary if the core post-excerpt block had a PHP filter on the return value of its render function, but I am not holding my breath that this would ever be considered by the core team.
b. It only works for two cases: when post-excerpt belongs to the template, and when post-excerpt belongs to a pattern that's called from the template. There are many other ways a theme could be built. (wp:template-part, nested patterns, reusable blocks, etc.) Because of the specific order and technique WP uses to render HTML from each of these object types (see (a) above), we would need separate approaches for each.

All this being said, the current approach seems to work OK for the core block themes, which is probably as much as we can hope for. For anyone running a different block theme, they can fix the problem manually: change the block template to a post-content block.

@dcavins and @imath What do you think of all this craziness? Is this too weird to ship?

@imath
Copy link

imath commented Dec 19, 2023

@boonebgorges that's a challenge !

About

they can fix the problem manually: change the block template to a post-content block

In this case they should probably add a new archive-{bp-docs-post-type-name}.html instead of edtiting archive.html.

I don't know BP Docs enough, but an easy way to have more control on the output could be to use a BP Directory page and put a BP Docs loop inside a regular template part?

@boonebgorges
Copy link
Owner Author

In this case they should probably add a new archive-{bp-docs-post-type-name}.html instead of edtiting archive.html.

Yes, agreed. Is there a way to do this in the Editor or do you have to create it in your theme directory? Everything changes all the time :-/

I don't know BP Docs enough, but an easy way to have more control on the output could be to use a BP Directory page and put a BP Docs loop inside a regular template part?

When I first wrote this plugin, I wanted to take advantage of custom post types as much as possible, so I avoided using BP directories. Adding a custom BP directory was always a pain anyway - maybe it's easier now. In any case, I'd need to rewrite a bunch of the pagination and routing logic if I went this route. And it seems to me that this would mean putting more eggs into the "page.html" basket. BP's theme compatibility technique has always been a hack, albeit a very clever hack, and the advent of block themes suggests that it might be time to come up with a different kind of approach rather than doubling down on the_content filters.

@dcavins
Copy link
Collaborator

dcavins commented Dec 19, 2023

I've been testing 738-return-buffered-template-part and, despite how we feel about how hacky it is to be doing str_replace on content (yep, str_replace always makes me think that there should be a better filter to use somewhere else, whether it exists yet or not), it does allow at least some block themes to work better. While I don't think this is the final solution, it's better than the current state of the plugin, which doesn't work on any block themes, as far as I've been able to see.

I agree that the long-term solution is to generate a BP Docs Archive item. I'm confusing myself a bit, because previously while full-site editing in one of the themes, it seemed that I could customize an archive for a single post type, but I can't seem to find that functionality today (I can only customize the "all archives" view). I thought maybe we could insert our directory block into however the archive customizations are saved, but I'll have to find the per-type interface again and see how it's saved.

@boonebgorges
Copy link
Owner Author

@dcavins Agreed. This is better than nothing, and we can always yank it later if it poses problems. Let's ship it.

I see you're working on the branch today. Let me know when you're relatively satisfied, and we'll merge back to 2.2.x. I think that this merge should address all the tickets in the milestone at once: https://github.com/boonebgorges/buddypress-docs/milestone/25 Then I'll push out a Christmas Miracle of a release. 🎄

@dcavins
Copy link
Collaborator

dcavins commented Dec 19, 2023

Ah, that's amazing! There's a GH user with the username since.

Anyway, yes, those items in the milestone I think are all ready to go. Do you want to manage the merge to 2.2.1, then I can run through my tests again with everything integrated into a release-ish state?

Let the Christmas Miracle commence, even if the puppy we're gifting is a little goofy looking with supersized ears and stubby legs and not the thoroughbred puppy that's featured in Hallmark movies!

@boonebgorges
Copy link
Owner Author

Amazing. I've merged to 2.2.x. I'll spend some time running a few tests and writing up the readme. Let me know when you've finished your tests.

@boonebgorges boonebgorges modified the milestones: Some day, 2.2.1 Dec 19, 2023
@boonebgorges
Copy link
Owner Author

For bookkeeping purposes, I'm going to put this in the 2.2.1 milestone, and we'll mark it resolved after testing. Future improvements to block-theme integration will have to go in the 'some day' milestone.

@dcavins
Copy link
Collaborator

dcavins commented Dec 19, 2023

I've also merged in the required changes for the group tab problem. Ive tested with BP 11.4, BP 12, and BP 12 with BP Classic in TwentyNineteen through Twenty-Four, and the behavior is much improved (wider in the non-block theme, at least displays in the block themes) and groups Docs screens are available to non-admins.

Thanks for the updates!

@imath
Copy link

imath commented Dec 19, 2023

@boonebgorges I think you have to create it in your theme directory, so users should copy archive.html as archive-{bp-docs-post-type-name}.html and make sure it uses the post-content block instead of the excerpt.

@dcavins
Copy link
Collaborator

dcavins commented Dec 19, 2023

@boonebgorges I think you have to create it in your theme directory, so users should copy archive.html as archive-{bp-docs-post-type-name}.html and make sure it uses the post-content block instead of the excerpt.

I can't imagine most users being able to do that (being comfortable with creating a child theme and then manipulating the files on the server). I wonder if we could do the archive-{bp-docs-post-type-name}.html file generation--which would only "stick" if the site was using a child theme, of course. This would really only work if archive "templates" could be soft--stored in the db and not as a physical file--I think.

Thanks for thinking about this problem, @imath! It seems we've run into something that shouldn't be this hard to do in WP. 👯

boonebgorges added a commit that referenced this issue Dec 20, 2023
The technique previously introduced to support the Docs directory
in block themes worked because the `bp_doc` archive query successfully
matched at least one item. It's important that the query match at least
one item, otherwise the search for the `post-excerpt` string will never
succeed, since the query block will render to show a `query-no-results`
element instead. This limitation in the technique meant that the Create
page would not work in block themes, since the `post_type=bp_doc&create=1`
query would never match any items.

BuddyPress's theme compatibility layer intends to work around the
404/no-results issue by using `bp_theme_compat_reset_post()`. This
function resets WP's global query with a "dummy" post object, which
in traditional themes tricks the theme into thinking that the query
matches a result. The technique doesn't work properly in block themes,
because of load order: In a block theme using the `core/query` block
and the 'inherit' attribute, the query is accessed at the
`template_include` hook. This causes race conditions with BP's
"dependency" system that loads theme compatibility at 'bp_template_include'.

The workaround is to run the theme-compatibility reset immediately
before running the template content through `render_block()` in the Docs
theme compatibility layer. This ensures that the dummy post is set
at the time that WP renders the `core/query` block, avoiding "no results".

See #728.
@boonebgorges
Copy link
Owner Author

I think you have to create it in your theme directory,

@imath Yeah, I figured as much.

So I figured out that the same problem is occurring on the Create page as well. This uncovers a limitation in the previous technique, which has to do with the way that BP's theme compatibility layer works (or doesn't work) at the template_include hook. I've found a workaround and tried to spell out some of the details at 46ea36f, though my explanation there, as wordy as it is, is still not complete. Anyway, I am going to sleep on this and test again tomorrow.

@boonebgorges
Copy link
Owner Author

As a side note, the pattern that twentytwentyfour uses for archives has three columns, which looks terrible with Docs. See https://github.com/WordPress/twentytwentyfour/blob/df92472089ede6fae5924c124a93c843b84e8cbd/templates/archive.html#L8, https://github.com/WordPress/twentytwentyfour/blob/df92472089ede6fae5924c124a93c843b84e8cbd/patterns/posts-3-col.php#L25. This points toward the possibility that we could have some theme-specific customizations, similar to what we do with default-theme-specific stylesheets in BP. In the case of twentytwentyfour, this might mean swapping out posts-3-col with posts-1-col on the archive-bp_doc dummy template. I don't feel like building out this infrastructure right now since it would just be piling on top of the existing terribleness of str_replace() etc, but it might be something to explore in the future if we come up with a less bonkers way of handling the general problems with theme templates.

@boonebgorges
Copy link
Owner Author

Let's go with this and see if the sky falls.

@dcavins
Copy link
Collaborator

dcavins commented Dec 23, 2023

It's still better than it was! Even if Chicken Little might be right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants