Make WordPress Core

Opened 2 years ago

Last modified 5 weeks ago

#55437 reopened defect (bug)

Bugfix: Display correct theme in site editor

Reported by: ptahdunbar's profile ptahdunbar Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version: 5.9
Component: Themes Keywords: dev-feedback needs-patch gutenberg-merge
Focuses: ui, administration Cc:

Description

Scenario:
Child themes inherit template parts from the parent theme but on the site editor page, the "Added by" column defaults to displaying the child theme even though the template parts are inherited from the parent.

This creates confusion as to where the actual templates are located.

Patch:
https://github.com/ptahdunbar/WordPress/commit/25bbf034af03c9a4ea33a79d01f4d9c079750344

Probably should look into _build_block_template_result_from_post() as well.

Possibly related to [52062]

Attachments (1)

55437.diff (2.7 KB) - added by SergeyBiryukov 16 months ago.

Download all attachments as: .zip

Change History (19)

#1 @SergeyBiryukov
2 years ago

  • Component changed from Administration to Themes
  • Milestone changed from Awaiting Review to 6.0

This ticket was mentioned in Slack in #core by chaion07. View the logs.


2 years ago

#3 @costdev
2 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from 6.0 to 6.1
  • Version changed from trunk to 5.9

As we're approaching RC1 and this still needs testing and unit tests, I'm moving this ticket to the 6.1 milestone and adding needs-unit-tests.

#4 @poena
19 months ago

Testing instructions

-Activate a child theme of a block theme. (example https://wordpress.org/themes/geologist/)
-In the WordPress admin area, go to Appearance > Editor. In the Site Editor, open the navigation sidebar and select Templates.

This will display a list of user created templates and templates in the parent theme and child theme.

Testing results

With the patch applied:
Parent theme templates are listed as added by the parent theme,
Child theme templates are listed as added by the child theme.

I noticed that the slugs are displayed first and that there is a delay before the text is changed to the theme name. This delay is unexpected, especially if the slug and name is not an exact match. for example, the slug may be "twentytwentythree" but the name is Twenty Twenty-Three.
I am not able to determine if the delay is related to the patch.

Last edited 18 months ago by poena (previous) (diff)

#5 @audrasjb
18 months ago

  • Milestone changed from 6.1 to 6.2

With WP 6.1 RC 1 scheduled tomorrow (Oct 10, 2022), there is not much time left to address this ticket. Let's move it to the next milestone.

Ps: if you were about to send a patch and if you feel it is realistic to commit it in the next few hours, please feel free to move this ticket back to milestone 6.1.

This ticket was mentioned in PR #3565 on WordPress/wordpress-develop by WoutPitje.


17 months ago
#6

In the site editor, the correct theme gets shown per template or template part.
https://i0.wp.com/user-images.githubusercontent.com/54243547/199973586-2ede40a7-a7e1-4c3f-a1f3-2d72ccca6f5d.png

Trac ticket: https://core.trac.wordpress.org/ticket/55437

#7 @SergeyBiryukov
16 months ago

  • Keywords has-unit-tests added; 2nd-opinion needs-unit-tests removed
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

Hi there, thanks for the ticket!

On a closer look, it appears that the change to _build_block_template_result_from_post() is not needed, as it already retrieves the correct theme from the post's associated terms in the wp_theme taxonomy.

55437.diff adds unit tests.

#8 @SergeyBiryukov
16 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 54860:

Site Editor: Show correct theme per template or template part.

Child themes inherit templates and template parts from the parent theme. In Site Editor, the "Added by" column for a template defaults to displaying the child theme, even though it is inherited from the parent, creating confusion as to where the actual templates are located.

This commit ensures that the parent theme is correctly displayed in that scenario.

Follow-up to [51003], [52062].

Props ptahdunbar, WoutPitje, petaryoast, costdev, poena, audrasjb, SergeyBiryukov.
Fixes #55437.

@SergeyBiryukov commented on PR #3565:


16 months ago
#9

Thanks for the PR! Merged in r54860.

#10 @hellofromTonya
13 months ago

  • Keywords dev-feedback added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening as [54860] broke saving changes in a parent's template part that is not in the active child theme's template parts. See #57630.

Why?

@azaozz and I think (and are assuming) $template_file (which is populated in _get_block_templates_files() is the actual filesystem location of each template part. So the 'theme' element is which theme has this template part.

In _build_block_template_result_from_file(), the later code using $template->theme expects it to be the active theme, not the theme where the template part lives.

What we don't understand is: How is $template->theme used? It seems there's a mismatch somewhere between how the Site Editor saves changes (#57630) and how the UI lists them (this ticket).

As the changeset has broken saving parent template parts, this changeset needs to be reverted for 6.2. And then a deeper review can occur in 6.3 to understand where the root cause(s) is(are). It is possible there is an underlying issue that is affecting both of these tickets.

I'm preparing the revert now.

#11 @hellofromTonya
13 months ago

In 55493:

Site Editor: Revert r54860.

[54860] caused a regression. Changes to a parent theme's template part (i.e.e when a child theme does not override that template part) no longer saved in the Site Editor. Reverting the changeset resolves the regression.

Props mreishus, hellofromTonya, azaozz, ironprogrammer, antonvlasenko.

Fixes #57630.
See #55437.

#12 @hellofromTonya
13 months ago

  • Milestone changed from 6.2 to 6.3

As noted in comment 10:

And then a deeper review can occur in 6.3 to understand where the root cause(s) is(are). It is possible there is an underlying issue that is affecting both of these tickets.

Moving this ticket to 6.3.

#13 @oglekler
9 months ago

  • Keywords needs-testing removed

Removing need-testing because it still needs dev-feedback

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


9 months ago

#15 @mukesh27
9 months ago

  • Keywords needs-patch added; has-patch has-unit-tests removed
  • Milestone changed from 6.3 to 6.4

This ticket was discussed during bug scrub.

No progress since last few months. Move to 6.4

Feel free to move in milestone it anyone have capacity for this.

#16 @hellofromTonya
8 months ago

  • Keywords gutenberg-merge added

Adding the gutenberg-merge keyword for tracking, as this bugfix might be better fixed and tested first within the Gutenberg repo.

#17 @hellofromTonya
5 months ago

  • Milestone changed from 6.4 to 6.5

6.4 RC1 is tomorrow. With no further progress, moving this ticket to 6.5.

#18 @swissspidy
5 weeks ago

  • Milestone changed from 6.5 to Future Release
Note: See TracTickets for help on using tickets.