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

Use a taxonomy instead of meta for template & template-part #24720

Closed
wants to merge 18 commits into from

Conversation

aristath
Copy link
Member

@aristath aristath commented Aug 21, 2020

As discussed in #24377 and a core editor meeting, we want to move away from using post-meta to store the theme, and start using a taxonomy instead.

This PR adds the taxonomy and changes all related queries to use the new wp_theme tax.

@aristath aristath added the [Type] Performance Related to performance efforts label Aug 21, 2020
@aristath
Copy link
Member Author

Terms now get properly created and assigned.
The queries seem to be working correctly.

@aristath aristath marked this pull request as ready for review August 25, 2020 14:53
@aristath aristath changed the title WIP: Use a taxonomy instead of meta for template & template-part Use a taxonomy instead of meta for template & template-part Sep 9, 2020
@aristath aristath marked this pull request as draft September 23, 2020 12:32
@aristath
Copy link
Member Author

On the PHP front this is OK, now working on the JS front.
Since this was imcomplete (I previously missed all the meta.theme references in the template-part JS files) I converted this PR to a draft so we can keep working on it.

@aristath aristath marked this pull request as ready for review September 28, 2020 09:30
@TimothyBJacobs
Copy link
Member

If this is using a taxonomy now, can we use the automated handling we'd get for querying and response inclusion by marking it as show_in_rest?

@aristath
Copy link
Member Author

aristath commented Oct 1, 2020

If this is using a taxonomy now, can we use the automated handling we'd get for querying and response inclusion by marking it as show_in_rest?

Thank you @TimothyBJacobs! I added show_in_rest to the taxonomy now, but I'm afraid I don't quite understand what you mean by "use the automated handling we'd get for querying and response inclusion", could you please elaborate?
Right now I'm exposing wp_theme in the JSON response using this: https://github.com/WordPress/gutenberg/pull/24720/files#diff-51f3ab0134282ae12e1994208a3ab6feR261, and then I use that instead of the meta we previously had. So templatePart.meta.theme just becomes templatePart.wp_theme. Are you suggesting there's a better way?

@TimothyBJacobs
Copy link
Member

Now that the taxonomy shows in REST, you should be able to query for it automatically using the rest_base of the taxonomy.

It should also now be automatically included in the response as the rest_base too.

@aristath
Copy link
Member Author

aristath commented Oct 2, 2020

@TimothyBJacobs
Ah OK, I see what you mean now.
The problem with the default responses however is that they use the term-ID, and what we have here is the term-slug.
So I don't think we'd be able to get what we need without an additional REST call... Instead of doing that, I simply added the term-slug in the response.
Note: after your comment I realized that there was a conflict with default responses. By default it was returning wp_theme: [ 2 ] so I renamed the slug to wp_theme_slug and it doesn't overwrite the wp_theme anymore in responses.

If you feel this can be done better then please feel free to push commits here, you are far more familiar with the REST API than I am!

@aristath
Copy link
Member Author

Superseded by #27016
Closing this one since the other PR is up to date with the latest structural changes and it will be easier to work on.

@aristath aristath closed this Nov 17, 2020
@aristath aristath deleted the aristath/try-24377 branch November 17, 2020 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants