Make WordPress Core

Opened 11 months ago

Closed 11 months ago

Last modified 11 months ago

#58405 closed enhancement (fixed)

Use is_block_theme in WP_Theme

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.3 Priority: normal
Severity: normal Version:
Component: Themes Keywords: good-first-bug has-patch needs-testing commit
Focuses: Cc:

Description (last modified by mukesh27)

In line of 366 / 367 of the WP_Theme class, there are manual calls to if files exists. However, in this context we can use the method is_block_theme. This remove repeated code and hit an existing cache.

Attachments (3)

58405.diff (638 bytes) - added by nihar007 11 months ago.
58405-2.diff (909 bytes) - added by nihar007 11 months ago.
Screenshot 2023-06-05 at 10.54.41.png (78.6 KB) - added by spacedmonkey 11 months ago.

Download all attachments as: .zip

Change History (21)

This ticket was mentioned in PR #4514 on WordPress/wordpress-develop by Juzar10.


11 months ago
#1

  • Keywords has-patch added
  • Added the is_block_theme check instead of the file_exists check for the WP_Theme

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

@nihar007
11 months ago

#2 @mukesh27
11 months ago

  • Description modified (diff)
  • Keywords changes-requested added

Hi there!

Thanks for PR/Patch both of you.

@nihar007 in your patch do you use different approach then PR 4514? Instead of adding duplicate patch try you can check review the PR and share your feedback on it. Thanks!

#3 @mukesh27
11 months ago

  • Description modified (diff)

#4 @spacedmonkey
11 months ago

  • Milestone changed from Future Release to 6.3

One of the two patches looks good. Moving into the 6.3 milestone.

To who is interested, this needs to be tested in classic and block themes, with and without child themes.

@nihar007
11 months ago

#5 @nihar007
11 months ago

Hi @mukesh27,
Thanks for you response.

I removed $theme_path variable. I think there is no necessity of $theme_path variable.

Juzar10 commented on PR #4514:


11 months ago
#6

Hi @mukeshpanchal27 ,
I have addressed the changes as suggested.
Thank you.

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


11 months ago

#8 @costdev
11 months ago

  • Keywords needs-testing added

This ticket was discussed during the bug scrub. We agreed to add the needs-testing keyword to indicate that the PR needs testing before commit consideration.

Additional props: @mukesh27 @oglekler

#9 @costdev
11 months ago

  • Keywords needs-testing-info added

@spacedmonkey From your earlier comment:

To who is interested, this needs to be tested in classic and block themes, with and without child themes.

Would you be able to add some testing instructions so that the Test Team can pick this up and cover possible Dashboard-based and code-based testing paths? Thanks!

@juzar commented on PR #4514:


11 months ago
#10

Hi @spacedmonkey,
I have made the changes you asked for.
Thank you.

@spacedmonkey commented on PR #4514:


11 months ago
#11

@mukeshpanchal27 @costdev Mind taking a look again?

#12 @spacedmonkey
11 months ago

@costdev To ensure this statement is hit, create a theme with just a style.css file. You should see an error message in the theme screen in admin. I have provided a screenshot.

@spacedmonkey commented on PR #4514:


11 months ago
#13

@costdev If you are happy, I will commit.

#14 @spacedmonkey
11 months ago

  • Keywords commit added; changes-requested needs-testing-info removed
  • Owner set to spacedmonkey
  • Status changed from new to assigned

@costdev If you can test, then I will commit.

#15 @costdev
11 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://patch-diff.githubusercontent.com/raw/WordPress/wordpress-develop/pull/4514.diff

Steps to Test

  1. Create a theme with only a valid style.css and no other files.
  2. Navigate to Appearance > Themes.
  3. Apply the patch.
  4. Refresh the page.

Environment

  • WordPress: 6.3-alpha-55505
  • PHP: 7.4.33
  • Server: Apache/2.4.56 (Ubuntu)
  • Database: mysqli (Server: 5.7.41-0ubuntu0.18.04.1 / Client: mysqlnd 7.4.33)
  • Browser: Chrome 114.0.0.0 (Windows 10/11)
  • Theme: Twenty Twenty-Three 1.1
  • MU-Plugins: None activated
  • Plugins: None activated

Expected Results

  • ✅ The following message should appear before and after the patch:

https://core.trac.wordpress.org/raw-attachment/ticket/58405/Screenshot%202023-06-05%20at%2010.54.41.png

Actual Results

  • ✅ The same message appeared before and after the patch.

@costdev commented on PR #4514:


11 months ago
#16

@spacedmonkey LGTM and tests well 👍

FYI, this patch brings an additional change (for the better, IMO):
trunk uses file_exists() rather than is_file(), so it's actually possible to create templates/index.html as a directory, and the "Broken Themes" message won't appear on the Appearances > Themes screen. With this PR, the "Broken Themes" message appears as expected.

#17 @spacedmonkey
11 months ago

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

In 55885:

Themes: Replace file_exists checks with call to is_block_theme method in WP_Theme class.

In WP_Theme class, replace two calls to file_exists with a call to the method is_block_theme. This method is_block_theme does the same file exists check, but it then caches the result for improved performance.

Props nihar007, spacedmonkey, mukesh27, costdev, juzar.
Fixes #58405.

Note: See TracTickets for help on using tickets.