WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#29618 closed enhancement (wontfix)

Improve `is_child_theme` to check for a valid Template value in child stylesheet comment header

Reported by: lancewillett Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch
Focuses: Cc:

Description

Currently is_child_theme() only verifies that template and stylesheet values don't match.

It'd be more accurate and safer to also verify that the child theme stylesheet also has a valid Template: [parentslug] value in it.

It's possible in situations where something goes wrong with a WP site or its options for template and stylesheet to not match without a child active. This extra check would avoid possible errors in cases like that, as well.

See a bit of history in #12998 and #20546.

Attachments (1)

29618.patch (724 bytes) - added by lancewillett 6 years ago.

Download all attachments as: .zip

Change History (8)

@lancewillett
6 years ago

#1 @nacin
6 years ago

This would trigger a filesystem hit. is_child_theme() was designed to be super simple, and deliberately wasn't converted over to WP_Theme. In fact, most functions don't use WP_Theme.

You can do if ( wp_get_theme()->parent() ) to see if WP_Theme thinks it has a parent, and also wp_get_theme()->parent()->errors() to see if the parent is broken.

#2 follow-up: @lancewillett
6 years ago

Counterpoint: without this suggested change, is_child_theme() isn't accurate. And misleading.

What are the performance concerns if WP_Theme is properly cached?

Can we instead add a method that is accurate to WP_Theme, then?

#3 in reply to: ↑ 2 @nacin
6 years ago

Replying to lancewillett:

What are the performance concerns if WP_Theme is properly cached?

None.

Can we instead add a method that is accurate to WP_Theme, then?

Yep, that's what parent() does.

#4 @lancewillett
6 years ago

I'm sensing a "No" here. I'll run with parent() but would prefer to have a nicely named wrapper that checks both template/stylesheet mismatch and the valid parent. To reduce complexity for everyday usage — and for new developers to know that is_child_theme() is actually accurate. What it says on the tin.

I don't think performance should be a reason to not improve the function, because high-traffic WP installs should use some sort of caching anyway.

Last edited 6 years ago by lancewillett (previous) (diff)

#5 @lancewillett
6 years ago

  • Keywords 2nd-opinion added

Would also love a 3rd opinion.

#6 @nacin
6 years ago

Always happy to have other opinions.

On a typical frontend load, WordPress does not actually perform any seeking operations from disk. The file headers for plugins, style.css, and translation PO files are only read when necessary, like when displaying a list of the available ones or when performing an update check.

Activated plugins are cached as an array of PHP files to include. The theme is cached as a few DB options, up to four: stylesheet, template, stylesheet_root, and template_root.

Hard drives can only do so many I/O operations per second. And that's assuming the disk is local. Hosts using NFS will not be super happy if we read the top of style.css every time trying to confirm a child theme really is a child theme.

I think that wp_get_theme()->parent() is nicely named, and it returns the WP_Theme object when there is a parent, or false otherwise. And for more advanced usage, if the theme has an invalid parent, then wp_get_theme()->errors()->get_error_code() will return "theme_no_parent". It's important to have these functions, but at the same time, it's important and helpful to be able to do things quickly. Most of the theme API does no filesystem verification, not just is_child_theme().

And this is for a mismatch that shouldn't really happen in practice. What's the practical need for this? Something can indeed go wrong, but in most cases we can account for that. Maybe the theme editor should ensure that if "Template:" either appeared or disappeared, that 'template' is up to date. (At least I don't think it does now.) Ultimately the fact that things go wrong is ultimately why wp-admin/themes.php has validate_current_theme(). (In fact, one reason we only do this on wp-admin/themes.php is because intermittent I/O failures used to potentially cause a site to reset to Kubrick. This is not fun.)

#7 @chriscct7
5 years ago

  • Keywords 2nd-opinion removed
  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.