WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 11 months ago

#20027 new enhancement

Use get_(template|stylesheet) instead of get_option( '(template|stylesheet)' )

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

Description

While working on Gandalf we noticed, that it's not enough to filter the current theme with

add_filter( 'template', array( $this, 'get_template' ) );
add_filter( 'stylesheet', array( $this, 'get_stylesheet' ) );

The reason are some uses of get_option( 'template' ) or get_option( 'stylesheet' ).

Means we need additional filters:

add_filter( 'pre_option_stylesheet', array( $this, 'get_stylesheet' ) );
add_filter( 'pre_option_template', array( $this, 'get_template' ) );

It would be handy if get_option( '(template|stylesheet)' ) could be replaced with get_(template|stylesheet).

Attachments (3)

20027.patch (6.9 KB) - added by ocean90 2 years ago.
20027.diff (3.2 KB) - added by nacin 2 years ago.
20027.2.patch (8.8 KB) - added by ocean90 2 years ago.

Download all attachments as: .zip

Change History (19)

ocean902 years ago

comment:1 scribu2 years ago

  • Keywords commit added; dev-feedback removed
  • Milestone changed from Awaiting Review to 3.4

No-brainer.

comment:2 follow-up: westi2 years ago

Should we deprecate the options too?

comment:3 ocean902 years ago

get_template() is more or less a wrapper for get_option('template') but has the filter in it.

comment:4 in reply to: ↑ 2 ; follow-ups: nacin2 years ago

Replying to westi:

Should we deprecate the options too?

Can't, as one calls the other.

I wonder if we should instead have never adopted get_stylesheet() and get_template(), and then filters could be placed on option_stylesheet and option_template. We could even maintain compat by applying the existing 'stylesheet' and 'template' filters on a callback for option_stylesheet and option_template, respectively.

It's not like these are theme template functions. The wrappers aren't that necessary and they only cause problems when filtering them.

comment:5 in reply to: ↑ 4 koopersmith2 years ago

Replying to nacin:

We could even maintain compat by applying the existing 'stylesheet' and 'template' filters on a callback for option_stylesheet and option_template, respectively.

This crossed my mind as well. If the only difference between the wrapper and the option is a single filter, that's an invitation for confusion and an open door for bugs. We should probably unify the two, alias the filters, and make the wrapper call get_option() and nothing else.

nacin2 years ago

comment:6 follow-up: nacin2 years ago

20027.diff hits at what I was thinking.

comment:7 in reply to: ↑ 6 SergeyBiryukov2 years ago

Replying to nacin:

20027.diff hits at what I was thinking.

  • Looks like option_stylesheet should be option_template in line 142.
  • Shouldn't it be add_filter() rather than add_action()?

comment:8 in reply to: ↑ 4 westi2 years ago

Replying to nacin:

Replying to westi:

Should we deprecate the options too?

Can't, as one calls the other.

Well we could but the code would be horrible (scanning backtraces on every call) so yes it isn't feasible.

I wonder if we should instead have never adopted get_stylesheet() and get_template(), and then filters could be placed on option_stylesheet and option_template. We could even maintain compat by applying the existing 'stylesheet' and 'template' filters on a callback for option_stylesheet and option_template, respectively.

It's not like these are theme template functions. The wrappers aren't that necessary and they only cause problems when filtering them.

I still think that the non-get_option calls are better ones to recommend people use and don't think that deprecating get_stylesheet or get_template is helpful.

I think we should encourage people to use get_stylesheet and get_template rather than get_option.

But we should make it so that they can't get different results.

The best way to do this is to make some changes to get_option and add some special cases for these two options for filtering - this is the simplest way to preserve the filter ordering that we have now (otherwise you have to guess at the largest priority something is hooked into the option filter and the filters in the wrapper functions lose out when they previously won)

comment:9 nacin2 years ago

Found something else while going through theme.php:

	// Prevent theme mods to current theme being used on theme being previewed
	add_filter( 'pre_option_mods_' . get_current_theme(), '__return_empty_array' );

We don't take into account pre_option_mods_{stylesheet} here, the newer mods storage key. I imagine this plays significantly into Gandalf do I don't want to make a change here. Doing get_option('stylesheet') (assuming it is not filtered) makes sense; I imagine you will be substituting in your own filter.

comment:10 nacin2 years ago

  • Keywords needs-refresh added

Let's keep get_option() separate from get_stylesheet() and get_template(), and go with the original patch proposed.

I say this because sometimes you might want the raw option. For example, we should not change the code in wp_update_themes() (wp-includes/update.php). This patch won't apply cleanly after #20103. Refresh and commit.

ocean902 years ago

comment:11 ocean902 years ago

  • Keywords commit needs-refresh removed

20027.2.patch

Not sure if the changes in wp-admin/includes/schema.php are useful.

comment:12 nacin2 years ago

The issue with the change in get_raw_theme_root() is two-fold:

  • One, it's designed to not be a filtered result.
  • Two, the get_option('stylesheet') and get_option('template') checks are within a branch where it also checks for get_option('stylesheet_root'). This means if someone is filtering only template or stylesheet, they will mess up the return value of the function as they are not accounting for stylesheet_root and template_root. So I'd almost rather rather leave it at get_option() and force customize to continue to filter both values. (Customize will likely need to disable update checks via wp_update_themes, though.)

Thoughts?

comment:13 ocean902 years ago

  • Milestone changed from 3.4 to Future Release

Since the Customizer handles both values, we can review this later.

comment:14 Rarst15 months ago

  • Cc contact@… added

comment:15 goto1015 months ago

  • Cc dromsey@… added

comment:16 lkraav11 months ago

  • Cc leho@… added
Note: See TracTickets for help on using tickets.