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

Site Editor: template parts not loading on Horizon #48145

Closed
vindl opened this issue Dec 8, 2020 · 11 comments
Closed

Site Editor: template parts not loading on Horizon #48145

vindl opened this issue Dec 8, 2020 · 11 comments
Assignees
Labels
[Goal] Full Site Editing [Pri] High Address as soon as possible after BLOCKER issues [Type] Bug

Comments

@vindl
Copy link
Member

vindl commented Dec 8, 2020

After latest Gutenberg plugin updates the template parts are no longer loading on FSE Horizon test flow.

  1. Go to https://horizon.wordpress.com/new?flags=gutenboarding/site-editor
  2. Follow up steps to create a new site.
  3. Notice the infinite spinners at the end when you get redirected to site editor. The header section is missing.

Screenshot 2020-12-08 at 23 57 51

@vindl vindl added [Type] Bug [Goal] Full Site Editing [Pri] High Address as soon as possible after BLOCKER issues labels Dec 8, 2020
@jeyip
Copy link
Contributor

jeyip commented Dec 8, 2020

Before Gutenberg v9.5.1 was patched the following logic caused problems with template part loading:

  • Template part post meta that's generated for theme provided template parts was returning an unexpected value. More specifically, wp_get_theme() -> get_stylesheet() was providing a theme string that looked like this pub/seedlet-blocks.
  • Client side code, however, was displaying a permanent spinner because:

In Gutenberg v9.5.1, David's recent PR modified the logic a bit (see here and here), but the call to wp_get_theme() -> get_stylesheet() is still implemented.

I suspect the cause of this issue is still the same.

@jeyip jeyip self-assigned this Dec 8, 2020
@jeyip
Copy link
Contributor

jeyip commented Dec 9, 2020

Hmm...I've confirmed that the cause is still identical in Gutenberg v9.5.1, but I'm having difficulty getting the rest wp template part query to return the correct data.

I've confirmed that the new filter_rest_wp_template_part_query is modifying the query properly. I've also confirmed that the correct template part exists in the database. Can't quite seem to figure out why the correct CPT isn't being returned, even if we're querying the taxonomy theme correctly with the pub prefix.

@jeyip
Copy link
Contributor

jeyip commented Dec 15, 2020

After lots of digging into api taxonomy query processing (great bedtime reading 😆 ), it turns out that an invalid taxonomy error was being swallowed. Long story short, the new wp_theme taxonomy, implemented here, wasn't being registered on time in a production environment.

@jeyip
Copy link
Contributor

jeyip commented Dec 15, 2020

D54315-code should fix the incorrect theme taxonomy registration
WordPress/gutenberg#27633 should fix querying template parts by the incorrect theme slug

@david-szabo97
Copy link
Contributor

New fix for incorrect theme slug: D54427-code

@jeyip
Copy link
Contributor

jeyip commented Jan 5, 2021

Status Update:

Ultimately, the issue has been traced to how we query for theme provided template parts. Ex. When themes have a parent directory like so parent-directory/my-theme, the theme taxonomy slug becomes parent-directory-my-theme. We, however, don't account for that. Instead, we fetch themes with a slug that looks like my-theme, which excludes any parent pathing.

There are three potential solutions in flight:

1. Modify the query made by the Gutenberg client

With this solution, we use the theme slug from the @wordpress/core-data store for the template parts query. This guarantees that we query by parent-directory-my-theme instead of just my-theme.

Pros

  • Should not affect existing FSE enabled sites

Cons

  • If we use the @wordpress/core-data theme slug, there is an edge case that Addison pointed out:
    1. User edits a theme supplied template and makes it a publish / custom version. But never edits the template-part auto-draft it references, so it is still saved by slug/theme and not Id.
    2. User then switches themes and uses their custom published template. Now the template part block assumes the auto draft is from the new theme, and the custom template loads an unexpected version of the template part?
  • There are ways around the edge case, but they require more work and a string of follow-up PRs

2. Modify the query in WPCOM (D54427-code)

Use a WordPress hook to intercept all theme taxonomy queries and theme taxonomy assignments to ensure the use of the theme slug's basename (i.e. my-theme and not parent-directory-my-theme)

Pros

  • No need to wait for the next Gutenberg release cycle

Cons

  • Potentially big performance concerns
  • Affects existing FSE enable sites (users will need to delete all existing theme provided templates and template parts)

3. Modify the query handled by Gutenberg backend logic

Ensure that, when theme provided template parts are initially created, they're always associated with the basename of the theme slug. That is to say, we always assign my-theme instead of parent-directory-my-theme when creating a new, auto-drafted template part. That way, our existing client side queries always work.

Pros

  • Straightforward to understand

Cons

  • Doesn't differentiate between identically named themes living under different parent directories (parent-1/my-theme vs. parent-2/my-theme)
  • Affects existing FSE enable sites (users will need to delete all existing theme provided templates and template parts)

@Addison-Stavlo
Copy link
Contributor

Addison-Stavlo commented Jan 6, 2021

A couple thoughts on each approach:

for Approach 1 - While the current state of @youknowriad's PR to get rid of auto-drafts makes it hard to distinguish between uncustomized/customized TPs which could make this approach less actionable, he does propose an idea of using the theme attribute ONLY on saved TPs. I wonder about that first edge case noted and if we could handle this by - whenever a template is customized/saved we force all template part blocks it references to be updated/saved as well. 🤔

for Approach 2 - Adding another Con - its fragile to different development patterns. If a request is written to retrieve a TP from a specific theme, it works! However, if any request is made for all template parts and is then filtered or analyzed in any other way on the front end there would still be that mismatch between directory/non-directory versions and results will not show up as expected.

Also noting this has the same con as 3 - doesn't differentiate between themes of the same name in differing directories (although that may not be a big issue for dotcom specifically).

for Approach 3 - this does seem the most simplistic and robust of the noted options. I would note the first con in a different light as well and that is changing/deviating from historical usage of how a theme "id" is represented in WordPress. Since including directories has always been done in the past (and thus allowed for separate themes of identical names), changing it specifically for FSE could open up problems and confusion for folks down the road.

@jeyip
Copy link
Contributor

jeyip commented Jan 6, 2021

I wonder about that first edge case noted and if we could handle this by - whenever a template is customized/saved we force all template part blocks it references to be updated/saved as well. 🤔

Interesting point. I'll have to give this more thought. 🤔

for Approach 2 - Adding another Con - its fragile to different development patterns.

for Approach 3 - this does seem the most simplistic and robust of the noted options. I would note the first con in a different light as well and that is changing/deviating from historical usage of how a theme "id" is represented in WordPress.

Great callout! Totally agree. I'm planning to make a collaborative google doc we can modify together, and I'll be sure to include these points.

@jeyip
Copy link
Contributor

jeyip commented Jan 14, 2021

When #28088 is merged, we still have to remove theme attributes from theme provided block templates per our discussions here. This will be done in follow-up PRs.

@jeyip
Copy link
Contributor

jeyip commented Feb 5, 2021

@ianstewart template parts should be loading again on FSE enabled sites for seedlet-blocks and tt1-blocks themes

@vindl
Copy link
Member Author

vindl commented Feb 5, 2021

Closing this since it works for both themes now.

@vindl vindl closed this as completed Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Goal] Full Site Editing [Pri] High Address as soon as possible after BLOCKER issues [Type] Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants