WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 3 months ago

Last modified 4 weeks ago

#46900 closed defect (bug) (fixed)

Site Health: count of inactive themes counts default and active themes separately, even when identical

Reported by: johnpgreen Owned by: azaozz
Milestone: 5.2 Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: site-health needs-testing has-patch
Focuses: Cc:

Description

I have nine themes installed in a development environment (the nine Twenty* themes) and Twenty Nineteen is the active theme. The Site Health Status page reports the following:

Your site has 9 installed themes, and they are all up to date.

Your site has 7 inactive themes, other than Twenty Nineteen, your active theme. We recommend removing any unused themes to enhance your site’s security.

It should report that there are 8 inactive themes.

See: https://github.com/WordPress/health-check/issues/205

Attachments (7)

46900.patch (544 bytes) - added by sudhiryadav 3 months ago.
46900.diff (603 bytes) - added by ianbelanger 3 months ago.
First pass at fixing issue
46900.2.diff (607 bytes) - added by mukesh27 3 months ago.
Updated patch.
46900-after-patch-no-child-theme.PNG (15.6 KB) - added by ianbelanger 3 months ago.
46900-after-patch-using-child-theme.PNG (16.4 KB) - added by ianbelanger 3 months ago.
46900-after-patch-using-twentynineteen-child-theme.PNG (17.1 KB) - added by ianbelanger 3 months ago.
46900.3.diff (854 bytes) - added by azaozz 3 months ago.

Download all attachments as: .zip

Change History (19)

#1 @desrosj
3 months ago

  • Keywords site-health needs-patch added
  • Milestone changed from Awaiting Review to 5.2

#2 @desrosj
3 months ago

  • Keywords good-first-bug added

@sudhiryadav
3 months ago

@ianbelanger
3 months ago

First pass at fixing issue

#3 @ianbelanger
3 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

My approach is a little different than @sudhiryadav. I'll leave it up to everyone to test and choose the best solution.

#4 @mukesh27
3 months ago

  • Keywords 2nd-opinion added

Both patch works fine for me but i think we have to go with @ianbelanger solution/patch as it will remove the code which we do not needed if we set $allowed_theme_count = 1; and it was already define in file so remove condition code is good solution for this ticket

#5 @Clorith
3 months ago

  • Keywords needs-refresh added; has-patch 2nd-opinion removed

This is slightly simpler than removing anything.

We need that counter to accommodate default themes existing when a non-default is active, for the inactive themes that "shouldn't" be kept around.

// If there's a default theme installed and not in use, we count that as allowed as well.
if ( $has_default_theme && ! $using_default_theme ) {
        $allowed_theme_count++;
}

This should negate the unwanted behavior in the correct manner I believe, only increasing the counter if we're not actually using the default theme.

@mukesh27
3 months ago

Updated patch.

#6 follow-up: @ianbelanger
3 months ago

I tested the most recent patch and it does, sort of, fix the issue, but with 2 caveats.

When using a child theme where Twenty Nineteen is not the parent, the theme count is correct, but the other than twentynineteen, the default WordPress theme text is missing.

If you use a child theme, with Twenty Nineteen as the parent, the count and text is incorrect as well.

Screenshots coming

#7 in reply to: ↑ 6 ; follow-up: @azaozz
3 months ago

Replying to ianbelanger:

When using a child theme where Twenty Nineteen is not the parent, the theme count is correct, but the other than twentynineteen, the default WordPress theme text is missing.

Hm, may be missing something but the second screenshot you uploaded says just that: keep TwentyNineteen, your current active child theme and it's parent theme:

https://core.trac.wordpress.org/raw-attachment/ticket/46900/46900-after-patch-using-child-theme.PNG

If you use a child theme, with Twenty Nineteen as the parent, the count and text is incorrect

If I understand this right, we also need to check if an active child theme is using the default theme as parent. Then set $using_default_theme = true; to not count it twice.

Patch coming up.

@azaozz
3 months ago

#8 @azaozz
3 months ago

  • Keywords has-patch added; good-first-bug needs-refresh removed

In 46900.3.diff: Count the default theme as "in use" when it is the parent of an active child theme. Fixes themes counts and mentioning the default theme twice when suggesting which themes to keep: as parent and as a default.

#9 in reply to: ↑ 7 ; follow-up: @ianbelanger
3 months ago

Replying to azaozz:

When using a child theme where Twenty Nineteen is not the parent, the theme count is correct, but the other than twentynineteen, the default WordPress theme text is missing.

Hm, may be missing something but the second screenshot you uploaded says just that: keep TwentyNineteen, your current active child theme and it's parent theme:

Why I considered the second image to be incorrect is that it is missing the other than twentynineteen, the default WordPress theme text. When you are not using a child theme, or TwentyNineteen as your active theme, the other than twentynineteen, the default WordPress theme text is shown, but when using a child theme, whether it's a child theme of TwentyNineteen or not, that text is not shown.

I will test the current patch and report back.

#10 in reply to: ↑ 9 @azaozz
3 months ago

Replying to ianbelanger:

Yeah, the wording is a bit different when using a child theme but thinking it is pretty clear that the user needs to keep three themes:

You should keep ...., the WordPress default theme, ...., your current theme, and ...., its parent theme.

Looked at this again and think 46900.3.diff works as expected. Going to commit it and close the ticket. Feel free to reopen (or make another ticket) if there are other edge cases.

#11 @azaozz
3 months ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

In 45260:

Site Health: Fix count of inactive themes and the recommendation to remove them when the default theme is active or is a parent of the active child theme.

Props sudhiryadav, ianbelanger, mukesh27, azaozz.
Fixes #46900.

#12 @spacedmonkey
4 weeks ago

  • Component changed from Administration to Site Health
Note: See TracTickets for help on using tickets.