Make WordPress Core

Opened 20 months ago

Closed 20 months ago

Last modified 6 months ago

#59279 closed enhancement (fixed)

Unnecessarily check to see site is using a child theme in the theme functions.

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.4 Priority: normal
Severity: normal Version: 4.7
Component: Themes Keywords: has-patch
Focuses: performance Cc:

Description

Follow on from #58576 / [56357].

There are a number of functions methods that check to see if file exists in a child theme before falling back to parent theme. However these functions do not check to see if current theme. These functions include.

The function is_child_theme can not be used, as TEMPLATEPATH and STYLESHEETPATH, might be different from the result of get_stylesheet_directory() and get_template_directory(). These functions have filters.

Attachments (1)

Screenshot 2023-09-05 at 11.01.37.png (65.6 KB) - added by spacedmonkey 20 months ago.

Download all attachments as: .zip

Change History (14)

This ticket was mentioned in PR #5133 on WordPress/wordpress-develop by @spacedmonkey.


20 months ago
#1

  • Keywords has-patch added

Only check to see if file exists if the current theme is a child theme. is_child_theme can not be used as get_stylesheet_directory has a filter.

Trac ticket: https://core.trac.wordpress.org/ticket/59279#ticket

#2 @spacedmonkey
20 months ago

We could maybe include _get_block_template_file in this.

#3 @spacedmonkey
20 months ago

Brenchmark data.

PHP 7.4.33, WordPress develop docker env. Trunk - [56512].

Theme - 2021

Trunk PR
Response Time (median) 54.37 54.08
wp-load-alloptions-query (median) 0.56 0.55
wp-before-template (median) 20.24 20.1
wp-template (median) 29.57 29.29
wp-total (median) 49.86 49.56

Theme 2022

Trunk PR
Response Time (median) 131.74 128.89
wp-load-alloptions-query (median) 0.59 0.58
wp-before-template (median) 82.09 79.87
wp-template (median) 44.99 44.79
wp-total (median) 127.35 124.57

Blackfire see a 6% improvement ( on 2022 theme )
https://blackfire.io/profiles/compare/3caa6f25-0dea-4cd7-a545-4fef945deb0f/graph

This ticket was mentioned in Slack in #core-performance by spacedmonkey. View the logs.


20 months ago

#7 @spacedmonkey
20 months ago

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

In 56523:

Themes: Remove unnecessary check if file exists in the theme functions.

Previously, several functions and methods in themes api were designed to check for the existence of files in a child theme before falling back to the parent theme. However, these checks did not consider whether the current theme was a child theme or not, resulting in unnecessary file existence checks for non-child themes. Check to see if stylesheet directory matches the template directory before doing the file exists. This optimization helps reduce unnecessary file system access, as file existence checks can be resource-intensive in PHP.

The following functions and methods have been updated as part of this enhancement:

  • WP_Theme::get_file_path
  • get_theme_file_path
  • get_theme_file_uri

Props spacedmonkey, flixos90, sabernhardt, 10upsimon, mukesh27.
Fixes #59279.

This ticket was mentioned in PR #7626 on WordPress/wordpress-develop by @desrosj.


6 months ago
#9

After the changes in Core-59279, Compose files are manually passed to the Docker commands. If a docker-compose.override.yml file is present, it won't be utilized. This attempts to detect that and include the file in the list that is passed.

Trac ticket:

@desrosj commented on PR #7626:


6 months ago
#10

@xknown env variable for which part? Do you mean for the additional value of volumes in the old-php-mysql-84.override.yml file?

I just did some testing, and it seems that Docker fails when an empty value for a volume is supplied. The new override file is needed _in addition_ to the other two supplied in the default docker-compose.yml file, not instead of. So I'm not sure how to get it working with an environment variable with the limited variable substitution available.

@xknown commented on PR #7626:


6 months ago
#11

@xknown env variable for which part? Do you mean for the additional value of volumes in the old-php-mysql-84.override.yml file?

I just did some testing, and it seems that Docker fails when an empty value for a volume is supplied. The new override file is needed _in addition_ to the other two supplied in the default docker-compose.yml file, not instead of. So I'm not sure how to get it working with an environment variable with the limited variable substitution available.

No, I was just wondering if it'd make sense to add something like the snippet below to utils.js

if ( typeof process.env.DOCKER_COMPOSE_CONFIG_OVERRIDE !== 'undefined' ) {
  composeFiles = composeFiles + process.env.DOCKER_COMPOSE_CONFIG_OVERRIDE
}

@desrosj commented on PR #7626:


6 months ago
#12

No, I was just wondering if it'd make sense to add something like the snippet below to utils.js

if ( typeof process.env.DOCKER_COMPOSE_CONFIG_OVERRIDE !== 'undefined' ) {
  composeFiles = composeFiles + process.env.DOCKER_COMPOSE_CONFIG_OVERRIDE
}

Ah, I see! I think if we find out that people are supplying custom override files we could consider that. But I don't know that is likely to be the case.

We also currently recommend using that file name in the README file for working with MySQL < 8.0 on Apple silicon chips. An env variable would also require an additional step (create the file, update the variable). So I think I prefer trying this approach first.

Note: See TracTickets for help on using tickets.