Update: Make template names and descriptions dynamic. #43862
Conversation
jorgefilipecosta
force-pushed
the
add/dynamic-template-names
branch
from
5f8ac74
to
71f91ba
Compare
|
Size Change: -382 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
jorgefilipecosta
force-pushed
the
add/dynamic-template-names
branch
from
71f91ba
to
b62c98a
Compare
jorgefilipecosta
added
[Feature] Site Editor
jorgefilipecosta
force-pushed
the
add/dynamic-template-names
branch
5 times, most recently
from
a941a8b
to
d27b917
Compare
jorgefilipecosta
force-pushed
the
add/dynamic-template-names
branch
from
d27b917
to
4b5cf9c
Compare
|
Hmm, this didn't work as expected for me. After updating my 'Category 20' to 'Category 30', the template name seemed to revert to a slug:
It doesn't look like the template is associated with the category any more either as I am able to create a new template for that category:
I'm not sure if it's relevant, but noting that I'd already created a template for this category before checking out this PR. Edit: Seems to be very relevant. If I create a new category on this branch, then create a template, then update the category name, then the template name updates as expected. Do we need to account for existing templates? |
| break; | ||
| case 'single': | ||
| $single_matches = array(); | ||
| $regex = '/(' . implode( '|', array_values( get_post_types() ) ) . ')-(.+)/'; |
There was a problem hiding this comment.
I think this is problematic if we have post types that could contain in their slug part of other post types - example CPT post-something would be mixed with post.
There was a problem hiding this comment.
I updated the logic now we try all combination until we find a referenced entity (if we can).
| break; | ||
| case 'taxonomy': | ||
| $taxonomy_matches = array(); | ||
| if ( preg_match( '/(' . implode( '|', array_values( get_taxonomies() ) ) . ')-(.+)/', $slug_remaining, $taxonomy_matches ) ) { |
There was a problem hiding this comment.
We probably have the same issue with the post types I commented about.
| switch ( $type ) { | ||
| case 'author': | ||
| $nice_name = $slug_remaining; | ||
| $users = get_users( |
There was a problem hiding this comment.
I haven't checked the query well here, but in my testing when I updated a nicename of a contributor user, it didn't find the user.. Can you reproduce?
Probably fine since this feature isn't in production yet. |
jorgefilipecosta
force-pushed
the
add/dynamic-template-names
branch
2 times, most recently
from
df56b04
to
abc648d
Compare
|
Hi @ntsekouras, @jameskoster thank you for the reviews. I guess for existing templates, we should not do anything. Making the titles and descriptions dynamic for templates created before this change would mean totally ignoring explicitly stored titles and descriptions, making what is stored in the database useless. I don't think we should do that, may in the future these fields will be needed for custom names etc. |
|
Yeah that sounds okay to me. Everything is working for me, so pending code review let's get this in! :) |
| * | ||
| * @param string $taxonomy Idenfitier of the taxonomy e.g:category. | ||
| * @param string $slug Slug of the term e.g:shoes. | ||
| * @param Gutenberg_Block_Template $template Template whose description and title are going to be computed. |
There was a problem hiding this comment.
| * @param Gutenberg_Block_Template $template Template whose description and title are going to be computed. | |
| * @param WP_Block_Template $template Template whose description and title are going to be computed. |
jorgefilipecosta
force-pushed
the
add/dynamic-template-names
branch
4 times, most recently
from
a1a75e1
to
74e63b2
Compare
| if ( empty( $posts ) ) { | ||
| $template->title = sprintf( | ||
| // translators: Represents the title of a user's custom template in the Site Editor referencing a deleted post, where %1$s is the singular name of a post type and %2$s is the slug of the deleted post, e.g. "Deleted: Page(hello)". | ||
| __( 'Deleted: %1$s(%2$s)', 'gutenberg' ), |
There was a problem hiding this comment.
Since we're using the slug, this message will be also shown if a user updates the slug. I think these are more edge cases, but it might make sense to update to Not found or something? 🤔
ntsekouras
dismissed
anton-vlasenko’s stale review
Thanks for the review Anton! All your suggestions were applied.
jorgefilipecosta
force-pushed
the
add/dynamic-template-names
branch
from
74e63b2
to
2df51d1
Compare
|
I just cherry-picked this PR to the wp/6.1 branch to get it included in the next release: 6680737 |
ockham
removed
the
Backport to WP 6.7 Beta/RC
After discussions with the Editor Tech leads for 6.1(@bernhard-reiter, @cbravobernal, and @ndiego) and additional guidance from @hellofromtonya, the decision has been made to exclude [WordPress/gutenberg#43862 Gutenberg PR #43862] from the pre-Beta 2 Gutenberg sync PR and revert [54280]. Why? [54280] added the feature's PHP code, but the JS package updates were not included before feature freeze(Beta 1), meaning the feature was incomplete. As the PHP code does not work without the JS package update, the feature is incomplete and missed the feature freeze deadline. Leaving the PHP code was discussed. However, there is a risk of it needing to change which could complicate backwards compatibility in 6.2 when the feature is eventually introduced. Follow-up to [54280]. Props hellofromTonya, bernhard-reiter, cbravobernal. See #56467. git-svn-id: https://develop.svn.wordpress.org/trunk@54333 602fd350-edb4-49c9-b593-d223f7449a82
After discussions with the Editor Tech leads for 6.1(@bernhard-reiter, @cbravobernal, and @ndiego) and additional guidance from @hellofromtonya, the decision has been made to exclude [WordPress/gutenberg#43862 Gutenberg PR #43862] from the pre-Beta 2 Gutenberg sync PR and revert [54280]. Why? [54280] added the feature's PHP code, but the JS package updates were not included before feature freeze(Beta 1), meaning the feature was incomplete. As the PHP code does not work without the JS package update, the feature is incomplete and missed the feature freeze deadline. Leaving the PHP code was discussed. However, there is a risk of it needing to change which could complicate backwards compatibility in 6.2 when the feature is eventually introduced. Follow-up to [54280]. Props hellofromTonya, bernhard-reiter, cbravobernal. See #56467. Built from https://develop.svn.wordpress.org/trunk@54333 git-svn-id: http://core.svn.wordpress.org/trunk@53892 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Update: I reverted this on the |
After discussions with the Editor Tech leads for 6.1(@bernhard-reiter, @cbravobernal, and @ndiego) and additional guidance from @hellofromtonya, the decision has been made to exclude [WordPress/gutenberg#43862 Gutenberg PR #43862] from the pre-Beta 2 Gutenberg sync PR and revert [54280]. Why? [54280] added the feature's PHP code, but the JS package updates were not included before feature freeze(Beta 1), meaning the feature was incomplete. As the PHP code does not work without the JS package update, the feature is incomplete and missed the feature freeze deadline. Leaving the PHP code was discussed. However, there is a risk of it needing to change which could complicate backwards compatibility in 6.2 when the feature is eventually introduced. Follow-up to [54280]. Props hellofromTonya, bernhard-reiter, cbravobernal. See #56467. Built from https://develop.svn.wordpress.org/trunk@54333 git-svn-id: https://core.svn.wordpress.org/trunk@53892 1a063a9b-81f0-0310-95a4-ce76da25c4cd
|
Won't there be backwards compatibility issues for folks who create (specific) templates between 6.1 and 6.2 if we leave this out? 🤔 |
|
I can't understand why we didn't include this with the next packages update and keep the backport. This is more like a bug than an enhancement. It would be good to have this for 6.1. |
|
I agree with what @ntsekouras said. This PR is not a feature; it is a bug fix, and we labeled it as such. |
|
I also agree with @ntsekouras and @jorgefilipecosta . This issue will likely cause bug reports (or, at least, confusion) when users rename their template and the old one is still being referenced. Even if this gets fixed later for 6.2, the stored data will still reference the wrong name, so I think it should be considered a bug. @ockham @ndiego, do you have more context on what led to consider this enhancement? |
|
@priethor I missed this discussion, but agree this is a bug. I can see how it can be thought of as an "enhancement", but it is also fixing a bug and can have a long-term impact. |
It was considered an incomplete feature rather than a bugfix, since the JS code ( Looping in @dream-encode and @hellofromtonya who discussed this with me -- in case y'all are open to re-considering 😊 |
|
@jorgefilipecosta @ockham @jameskoster @ntsekouras @priethor Thanks for the ping. I am open to reconsidering this for inclusion if this is truly a bug fixing unintended behavior already in trunk, but I have a question that @desrosj posed regarding the theme supports PR from yesterday.
Thanks in advance for the additional context! |
In 6.0 it's not possible to create templates for specific entities. So the issue this PR addressed doesn't exist in production, only in the Gutenberg plugin. We can consider it a part of #37407. |
|
@jameskoster Thanks! In chatting with @hellofromtonya and revisiting our thought process a couple of days ago in our review. Following the breadcrumb trail, this issue appears to be directly tied to #42894. That itself is an #42894 seeks to improve the template naming to prevent "confusion". However, unless we're missing something, I don't know that this addresses any broken functionality. Our interpretation from the Core side is that this was an incomplete feature. Reverting the PHP part of this PR removed unused code that was not actually consumed by the Editor after 6.1 Beta 1 was released. Finally, there could be a case for removing the feature in #42165 and any follow-ups if the feature is deemed not fully functional after Beta 1. We've not entertained that as yet, as we respect the hard work done on the feature that was merged in time for the Beta phase, and, in reading the trail of PRs, it seems usable without this enhancement. |
|
Hi @dream-encode, #42894 is marked as an enhancement, but I think it should be considered a bug, not an enhancement (this PR was labeled a bug since the creation).
If, when changing the title of the post, the post list still displayed the old title, and no update happened, I guess we would consider that a bug, right?
That PR is just one of the PRs of the feature, the feature encompasses many PRs for different entities and UI's and is deeply entrenched in the codebase. Removing the feature does not seems to be a good path. During the removal probably, more issues would be introduced. If we don't interpret this PR as a bug, we should leave things as they are and fix the problem in the next version. |
|
@jorgefilipecosta Thanks for the response! One thing to clarify is that this PR was marked as both enhancement and a bug, and I think I understand why now. If updating the post name doesn't update the template name, and that was the intended behavior all along, that is indeed a bug. However, I think this could've been considered "feature incomplete" when multiple templates for similar types was added. At best, this is straddling the line between bug and enhancement. I don't think there's any back-compat issues, as suggested above, as if this were to ship later, the templates added in the period before this merge would function just fine, even if their names were the same. That being said, your explanation has given me additional context and while re-reviewing the trace up issues leading to this, I think this improvement is one I'm willing to support as a "quality of life"/UX "fix" during Beta. I'm going to suggest some edits to the PR to bring it inline with the code reverted a few days ago and improve the docblocks for the new functions. Once the PR is ready, I am happy to merge this. I would just request a final confirmation from @ntsekouras and @jameskoster that tests in its final form yielded the expected results. |
There was a problem hiding this comment.
I think I covered all the unique issues I see, but I want to also point to the original core merge r54280 to ensure all the tweaks there make it back to this PR as well.
| * @param string $post_type Post type e.g.: page, post, product. | ||
| * @param string $slug Slug of the post e.g.: a-story-about-shoes. | ||
| * @param WP_Block_Template $template Template to mutate adding the description and title computed. | ||
| * |
There was a problem hiding this comment.
Remove this extra line between params and return.
| * Builds the title and description of a post specific template based on the underlying referenced post. | ||
| * Mutates the underlying template object. | ||
| * | ||
| * @access private |
There was a problem hiding this comment.
Add a @since 6.1.0 above the access.
| * | ||
| * @return boolean Returns true if the referenced post was found and false otherwise. | ||
| */ | ||
| function _gutenberg_build_title_and_description_for_single_post_type_block_template( $post_type, $slug, WP_Block_Template $template ) { |
There was a problem hiding this comment.
Update the function prefixes from gutenberg to wp.
There was a problem hiding this comment.
I think the prefix has to be renamed in the backport PR, not the Gutenberg PR. This function will disappear from Gutenberg code when the minimum WP version for Gutenberg increases to 6.2
| @@ -248,6 +248,136 @@ function gutenberg_get_block_template( $id, $template_type = 'wp_template' ) { | |||
| return apply_filters( 'get_block_template', $block_template, $id, $template_type ); | |||
| } | |||
|
|
|||
| /** | |||
| * Builds the title and description of a post specific template based on the underlying referenced post. | |||
| if ( empty( $posts ) ) { | ||
| $template->title = sprintf( | ||
| // translators: Represents the title of a user's custom template in the Site Editor referencing a post that was not found, where %1$s is the singular name of a post type and %2$s is the slug of the deleted post, e.g. "Not found: Page(hello)". | ||
| __( 'Not found: %1$s(%2$s)', 'gutenberg' ), |
There was a problem hiding this comment.
Remove the gutenberg domain, as this will fallback to Core domain.
There was a problem hiding this comment.
I think the domain has to be removed in the backport PR, not the Gutenberg PR. This function will disappear from Gutenberg code when the minimum WP version for Gutenberg increases to 6.2
| ); | ||
| $template->description = sprintf( | ||
| // translators: Represents the description of a user's custom template in the Site Editor, e.g. "Template for Page: Hello". | ||
| __( 'Template for %1$s', 'gutenberg' ), |
There was a problem hiding this comment.
I think this can be just %s since there's only one replacement.
| } | ||
|
|
||
| /** | ||
| * Builds the title and description of a taxonomy specific template based on the underlying entity referenced. |
| * Builds the title and description of a taxonomy specific template based on the underlying entity referenced. | ||
| * Mutates the underlying template object. | ||
| * | ||
| * @access private |
| * @param string $slug Slug of the term, e.g.: shoes. | ||
| * @param WP_Block_Template $template Template to mutate adding the description and title computed. | ||
| * | ||
| * @return boolean True if an term referenced was found and false otherwise. |
| ); | ||
| if ( count( $users_with_same_name ) > 1 ) { | ||
| $template->title = sprintf( | ||
| // translators: Represents the title of a user's custom template in the Site Editor, where %1$s is the template title of an author template and %2$s is the nicename of the author, e.g. "Author: Jorge (jorge-costa)". |
There was a problem hiding this comment.
I think we want to change the example author here to something anonymous, like the other references.
Let's use "Jane Doe (jane-doe)"
Backports PHP changes in WordPress/gutenberg#43862 to the core. Adds a mechanism to dynamically compute names and descriptions of the author, page, single, tag, category, and taxonomy templates. Props mcsf, ntsekouras, antonvlasenko, jameskoster. See #56467. git-svn-id: https://develop.svn.wordpress.org/trunk@54280 602fd350-edb4-49c9-b593-d223f7449a82
After discussions with the Editor Tech leads for 6.1(@bernhard-reiter, @cbravobernal, and @ndiego) and additional guidance from @hellofromtonya, the decision has been made to exclude [WordPress/gutenberg#43862 Gutenberg PR #43862] from the pre-Beta 2 Gutenberg sync PR and revert [54280]. Why? [54280] added the feature's PHP code, but the JS package updates were not included before feature freeze(Beta 1), meaning the feature was incomplete. As the PHP code does not work without the JS package update, the feature is incomplete and missed the feature freeze deadline. Leaving the PHP code was discussed. However, there is a risk of it needing to change which could complicate backwards compatibility in 6.2 when the feature is eventually introduced. Follow-up to [54280]. Props hellofromTonya, bernhard-reiter, cbravobernal. See #56467. git-svn-id: https://develop.svn.wordpress.org/trunk@54333 602fd350-edb4-49c9-b593-d223f7449a82
Fixes: #42894
Makes template names and descriptions of the author, page, single, tag, category, and taxonomy dynamic.
What?
Currently, we are storing stating names and descriptions in templates based on the title or name of the underlying entity at a given moment. That approach is not ideal if the user changes titles or names, the templates keep the old titles which may become very confusing.
This PR fixes the issue by making template titles and descriptions dynamic and computed on the server.
How?
When creating a new template for the author, single, tag, category, and taxonomy, instead of specifying a title and a description, we fill the slug as the title and keep the description empty.
On the server template object building, we added logic that when the template title equals the slug or is empty and the description is empty, we dynamically compute a human-friendly title and description.
We are making the title equal to the slug instead of being empty because there is logic to disallowing creating posts with an empty title description and content. If there was no fallback content on the template and the title was empty, it would fall under these rules, and the creation would fail.
Testing Instructions
Created a template specific for a post, a tag, a category, a custom taxonomy term, and an author.
Changed the titles of the post, tag, category, custom taxonomy term, and author, and verified the template title and description also updated.