WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 16 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 dev-feedback
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 (4)

20027.patch (6.9 KB) - added by ocean90 6 years ago.
20027.diff (3.2 KB) - added by nacin 6 years ago.
20027.2.patch (8.8 KB) - added by ocean90 6 years ago.
20027.3.patch (6.1 KB) - added by sebastian.pisula 16 months ago.

Download all attachments as: .zip

Change History (23)

@ocean90
6 years ago

#1 @scribu
6 years ago

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

No-brainer.

#2 follow-up: @westi
6 years ago

Should we deprecate the options too?

#3 @ocean90
6 years ago

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

#4 in reply to: ↑ 2 ; follow-ups: @nacin
6 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.

#5 in reply to: ↑ 4 @koopersmith
6 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.

@nacin
6 years ago

#6 follow-up: @nacin
6 years ago

20027.diff hits at what I was thinking.

#7 in reply to: ↑ 6 @SergeyBiryukov
6 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()?

#8 in reply to: ↑ 4 @westi
6 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)

#9 @nacin
6 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.

#10 @nacin
6 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.

@ocean90
6 years ago

#11 @ocean90
6 years ago

  • Keywords commit needs-refresh removed

20027.2.patch

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

#12 @nacin
6 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?

#13 @ocean90
6 years ago

  • Milestone changed from 3.4 to Future Release

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

#14 @Rarst
5 years ago

  • Cc contact@… added

#15 @goto10
5 years ago

  • Cc dromsey@… added

#16 @lkraav
5 years ago

  • Cc leho@… added

#17 @chriscct7
3 years ago

  • Keywords dev-feedback added

Any updated thoughts on this?

#18 @ocean90
23 months ago

#37352 was marked as a duplicate.

#19 @sebastian.pisula
16 months ago

Patch for 4.8-alpha-40051

Note: See TracTickets for help on using tickets.