WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#22515 closed defect (bug) (fixed)

Child theme installation not installing parent (regression)

Reported by: Otto42 Owned by:
Milestone: 3.4.3 Priority: low
Severity: normal Version:
Component: Upgrade/Install Keywords: has-patch
Focuses: Cc:

Description

Somewhere, the theme install process for child themes causing an install of the parent got broken. Not sure where.

This seems to be caused by the WP_Theme class quitting early when the parent theme is not installed, and not properly setting the parent information in the object. Moving the block of code that sets the parent info up a bit in the constructor fixes it.

Patch attached.

Related #13774

Attachments (2)

22515.diff (3.4 KB) - added by Otto42 2 years ago.
move parent detection code up in the constructor
22515.2.diff (3.1 KB) - added by nacin 2 years ago.

Download all attachments as: .zip

Change History (13)

@Otto422 years ago

move parent detection code up in the constructor

comment:1 @nacin2 years ago

  • Milestone changed from Awaiting Review to 3.4.3

I reproduced this in the 3.4 branch.

We need to figure out when this worked in 3.4, and what broke it.

comment:2 @chipbennett2 years ago

  • Cc chip@… added

comment:3 @sabreuse2 years ago

The patch fixes the issue for me in both 3.4 and trunk.

comment:4 @SergeyBiryukov2 years ago

Works in [20267], broken literally 10 minutes later in [20268].

If parent theme doesn't exist yet, WP_Theme constructor bails in line 256, before $this->parent is set:
http://core.trac.wordpress.org/browser/tags/3.4.2/wp-includes/class-wp-theme.php#L241

check_parent_theme_filter() then bails in line 683 due to $theme_info->parent() being empty:
http://core.trac.wordpress.org/browser/tags/3.4.2/wp-admin/includes/class-wp-upgrader.php#L678

comment:5 @nacin2 years ago

ಠ_ಠ

@nacin2 years ago

comment:6 @nacin2 years ago

If you look at how Theme_Upgrader works, it specifically checks if the theme is already installed by checking for $theme->parent()->errors(). That's the key. The parent theme is potentially valid, it just doesn't exist.

This is different from being a completely invalid parent, such as being a child of yet another theme, or suggesting that its own child is the parent (circular reference). In those cases, we do set parent to null (line 265), because the theme itself is broken and its parent is never going to be valid.

22515.diff happens to work in this case, but the block is getting moved up above code it depends on (see $theme_root_template, for example). 22515.2.diff makes sure we set $this->parent in this one case. Fixes everything for me.

Additionally, the ! parent()->errors() check in Theme_Upgrader should actually become a parent()->exists() check. That's because the parent() WP_Theme object will properly get theme_not_found as the WP_Error code. If there happens to be an error in the theme, we still don't want to try installing it because it *does* exist. (A future enhancement could be to try to upgrade/replace a parent theme before installing a child theme of it, especially if it is broken. This will not happen implicitly now because clear_destination => false.)

The only remaining issue is that a broken theme can still technically be activated. That's no fun, though. 22515.2.diff additionally guards against that one issue.

Only the single-line WP_Theme change is necessary for the 3.4 branch to get things working again. The rest is poka yoke.

comment:7 @nacin2 years ago

In 22784:

WP_Theme: If the parent theme is missing, instantiate a WP_Theme object anyway, so it can hold errors.

Fixes the installation of parent themes when installing a child theme from WordPress.org.

see #22515.

comment:8 @nacin2 years ago

  • Priority changed from high to normal
  • Severity changed from major to normal

[22784] fixes the main issue at play here. I may avoid the rest of 22515.2.diff until 3.6. Can we get some testers?

comment:9 @nacin2 years ago

  • Priority changed from normal to low

A bunch of people have verified this: WraithKenny, sabreuse, nofearinc. The regression is no longer present, so moving to low priority.

comment:10 @nacin2 years ago

In 22862:

WP_Theme: If the parent theme is missing, instantiate a WP_Theme object anyway, so it can hold errors.

Fixes the installation of parent themes when installing a child theme from WordPress.org.

see #22515.
for the 3.4 branch.

comment:11 @nacin2 years ago

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

I think it is worth closing this as fixed. The extra error handling can be added in 3.6, and if necessary, backported to the 3.5 branch. Opened #22602 for that.

Note: See TracTickets for help on using tickets.