WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#20313 closed defect (bug) (fixed)

get_theme_data can cause invalid WP_Theme object data to be persistantly cached

Reported by: westi Owned by: nacin
Milestone: 3.4 Priority: high
Severity: major Version: 3.4
Component: Themes Keywords: has-patch close
Focuses: Cc:

Description

The new version of get_theme_data in trunk is not aware of the concept of themes in subdirectories and makes the naive assumption that the style.css file it is passed to parse is for a theme in the root of a registered theme root rather than in a subdirectory.

The function itself works fine and returns correct data (Tests in [UT591]) but when a persistent caching back end is enabled the WP_Theme object caches data with incorrect stylesheet and template values for the theme.

For example if we have a theme in wp-content/themes/awesome/amazing-theme/ then both stylesheet and template should point to awesome/amazing-theme but if you call get_theme_data with the full path to the style.css file in wp-content/themes/awesome/amazing-theme/ the WP_Theme object gets created for the theme slug amazing-theme with the theme root wp-content/themes/awesome//.

Because the cache keys are based on md5sum of the path to the theme folder this data is then cached for the awesome/amazing-theme theme in the wp-content/themes/ theme root.

Attached patch fixes this for me in my testing (using error_logs inside get_theme_data to examine the WP_Theme object.)

I tried writing unit tests to show the badly cached data but it wasn't easy to do :(

Attachments (1)

get_theme_data-subdirs-fix.diff (761 bytes) - added by westi 3 years ago.
Fix to the logic in get_theme_data to correctly determine the theme slug and root to create a correct WP_Theme object.

Download all attachments as: .zip

Change History (6)

@westi3 years ago

Fix to the logic in get_theme_data to correctly determine the theme slug and root to create a correct WP_Theme object.

comment:1 @nacin3 years ago

Yeah, I definitely see what went wrong here. Nice catch.

new WP_Theme( 'twentyten', WP_CONTENT_DIR . '/themes/pub' ) should not work... It should recognize that the theme root is not a valid one and that the stylesheet should instead be twentyten/pub. Either it should error out by saying the theme does not exist, or it should correct the theme root and stylesheet on the fly to reflect the proper placement of /pub.

I will take a closer look at the patch -- not sure it will always work due to the finickiness of get_theme_root().

Yeah, testing the cached data can be tough. cache_delete() is a public method, so that could be used to test before/after situations.

comment:2 @nacin3 years ago

In [20316]:

Correct the values for theme_root and stylesheet when the values passed to the WP_Theme constructor have a directory appended to the theme root when it should actually be prepended to the stylesheet (when the theme is in a directory of themes inside a theme root). see #20313.

comment:3 @nacin3 years ago

  • Keywords close added

get_theme_data-subdirs-fix.diff uses get_theme_root(). For reference, it was a design decision for the WP_Theme API to use raw, unfiltered values as much as possible. (The only situation where it does not is theme root URIs.) Hence, get_raw_theme_root() would be a better choice here.

I looked at the problem and think [20316] is the best solution. I do remember testing this exact situation but I neglected to account for it in the end. It is something that WP_Theme should handle, given that after immersing myself in this for the entire month of March, I still made that mistake when deprecating get_theme_data().

So, if WP_Theme is going to handle it, I think it is easiest if get_theme_data() just relies on the behavior.

comment:4 @westi3 years ago

I think your solution should work fine.

I added some tests in [UT598] for the bug I saw and it fixes it perfectly.

comment:5 @nacin3 years ago

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.