Make WordPress Core

Opened 12 years ago

Last modified 12 months ago

#18298 reopened enhancement

deprecate TEMPLATEPATH and STYLESHEETPATH

Reported by: aaroncampbell's profile aaroncampbell Owned by: wonderboymusic's profile wonderboymusic
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Themes Keywords: needs-patch
Focuses: template Cc:

Description (last modified by aaroncampbell)

As part of #15086 we wanted to add unit tests, but we came across a problem in #UT22. Basically locate_template() uses these constants instead of get_template_directory() and get_stylesheet_directory() which makes it impossible to use switch_theme(). It looks like the constants are only used in about 20 places through core, so I recommend we fix those places and deprecate the constants.

Attachments (4)

18298.diff (3.7 KB) - added by aaroncampbell 12 years ago.
18298.2.diff (4.3 KB) - added by aaroncampbell 12 years ago.
18298.3.diff (4.4 KB) - added by aaroncampbell 12 years ago.
18298.4.diff (4.7 KB) - added by obenland 9 years ago.

Download all attachments as: .zip

Change History (45)

#1 @scribu
12 years ago

20 places isn't exactly few, but I agree that functions are better than constants, in this case.

#2 @lloydbudd
12 years ago

deprecate the constants

This would be very well received by the WordPress.com VIP team. We are regularly cleaning up TEMPLATEPATH and STYLESHEETPATH, replacing with the get_ functions in VIP partners themes' function to ensure features like mobile theme plugins and post-by-email work which don't go through initialization of those constants.

#3 @toscho
12 years ago

  • Cc info@… added

@aaroncampbell
12 years ago

#4 @aaroncampbell
12 years ago

  • Keywords has-patch added; needs-patch removed

The patch removes all core uses of the constants. It's still used in twentyten and twentyeleven, so those should be fixed as well.

I'm not sure how we want to deprecate constants, so I just left them there. If we don't have any way in place to actually deprecate them, we should at LEAST add comments that say they're deprecated.

#5 @scribu
12 years ago

Yeah, it seems the practice is simply to add a @deprecated tag. Example: PLUGINDIR

#6 @aaroncampbell
12 years ago

18298.2.diff adds the @deprecated tag to the constants.

#7 @cais
12 years ago

Following along the lines of the @since documentation would there be a general benefit to having the version when the item is deprecated included as well?

For example, from this ticket's premise, @deprecated 3.3

#8 @chipbennett
12 years ago

  • Cc chip@… added

Shouldn't these constants have version/replacement function information in the @deprecated phpDoc tag? (I ask mainly so that the various development tools, such as the Log Deprecated Notices Plugin and the Theme Check Plugin can be configured to pick up this information, to pass along to developers.

#9 @aaroncampbell
12 years ago

18298.3.diff adds php doc like this:

 * @deprecated 3.3
 * @deprecated Use get_stylesheet_directory()

#10 follow-up: @scribu
12 years ago

@chip: Log Deprecated Notices plugin can't do anything about it, since there's no mechanism in PHP to find out when a particular constant is used. Same for global variables (which is why they're evil).

#11 in reply to: ↑ 10 @chipbennett
12 years ago

Replying to scribu:

@chip: Log Deprecated Notices plugin can't do anything about it, since there's no mechanism in PHP to find out when a particular constant is used. Same for global variables (which is why they're evil).

Why is that? Couldn't either Plugin read the phpDoc headers, and cross reference the @deprecated tag against the constant being used anywhere in the Theme files? (Isn't that what they do anyway, to provide the deprecation (since-version, alternative function)?

#12 follow-ups: @aaroncampbell
12 years ago

I think that what he's saying is that you'd have to build some sort of regex to look for the constant being used since there's no way to actually throw a deprecated notice since it's not a function. For example, how do you find:

$test = $prefix . TEMPLATEPATH . $suffix;

but not

$test = $prefix . 'TEMPLATEPATH' . $suffix;
$test = "$prefix TEMPLATEPATH $suffix";
$test = <<<TEST
    $prefix
    Bad: TEMPLATEPATH
    $suffix
TEST;

#13 in reply to: ↑ 12 @chipbennett
12 years ago

Replying to aaroncampbell:

I think that what he's saying is that you'd have to build some sort of regex to look for the constant being used since there's no way to actually throw a deprecated notice since it's not a function.

Pross is the WPTRT's resident RegEx ninja; if anybody can figure out a way to do it, he can. :) So, maybe it's something that can find its way into Theme Check. My interest in the question (which now exceeds the scope of the patch itself) is in providing as much information as possible to Theme Developers, and getting them pre-emptively to begin to adopt the use of get_template_directory_*() and get_stylesheet_directory_*().

#14 in reply to: ↑ 12 ; follow-up: @toscho
12 years ago

Replying to aaroncampbell:

I think that what he's saying is that you'd have to build some sort of regex to look for the constant being used

Impossible. PHP is more complex than PCRE, so you’d have to use a tokenizer. Just think about constants in comments … ;)

#15 in reply to: ↑ 14 @chipbennett
12 years ago

Replying to toscho:

Replying to aaroncampbell:

I think that what he's saying is that you'd have to build some sort of regex to look for the constant being used

Impossible. PHP is more complex than PCRE, so you’d have to use a tokenizer. Just think about constants in comments … ;)

Which would be just fine for Theme Check. It ignores anything in PHP comments. Also, even if it finds false-positives, that's okay. Most such output is listed as INFO anyway: meant as a hint to the developer/reviewer to verify that its usage is proper.

In any case, this rabbit trail has strayed far from the intent of the ticket. I'll take it offline with Pross, and if he comes up with something viable for Theme Check, then I will take a look at the rest of the defined constants with @deprecated, to see if any could benefit from additional deprecation information, and submit a separate patch.

#16 @nacin
12 years ago

  • Type changed from defect (bug) to enhancement

#17 @aaroncampbell
12 years ago

  • Description modified (diff)
  • Version set to 3.0

#18 @rmccue
11 years ago

  • Cc me@… added

#19 @aaroncampbell
11 years ago

  • Milestone changed from Awaiting Review to Future Release

#20 @mordauk
11 years ago

  • Cc pippin@… added

#21 @omarabid
11 years ago

  • Cc omarabid added

Any updates on this. This ticket has over a year, and yet the Constants are still used instead of the function.

The file is question is: wp-includes/template.php

The Constants are used in the locate_template() function. This makes it impossible to change the location of my template files. For a special App. I'm building, I need to change these locations.

This is possible, but only if the functions were used instead of the Constant.

Is there a reason to keep the Constants instead? I have changed my WordPress Core code (which is bad), shall I submit my modification.

#22 @wpsmith
11 years ago

  • Cc travis@… added

#23 @juliobox
10 years ago

  • Cc juliobosk@… added

#24 @digitalquery
10 years ago

  • Cc anu@… added

#25 @wycks
10 years ago

  • Cc bob.ellison@… added

#26 @CrazyJaco
9 years ago

Also checking in for an update on this.

I have a similar need as omarabid.

It looks like its been 2 years since there was any sort of activity on this. Is this still on anyone's radar?

Thanks

#27 @DrewAPicture
9 years ago

  • Focuses template added
  • Keywords needs-refresh added
  • Milestone changed from Future Release to 4.0

I think we could take another look at this. Moving to 4.0 for consideration.

@obenland
9 years ago

#28 @obenland
9 years ago

  • Keywords needs-refresh removed

#29 @wonderboymusic
9 years ago

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

In 28563:

Replaces all uses of TEMPLATEPATH and STYLESHEETPATH in core with get_template_directory() and get_stylesheet_directory().

Add @deprecated annotations to TEMPLATEPATH and STYLESHEETPATH definitions.

Props obenland, aaroncampbell.
Fixes #18298.

#30 @DrewAPicture
9 years ago

In 28569:

Use three-digit x.x.x style version for @deprecated phpDoc tags.

See #18298.

#31 @nacin
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I've always thought this was a bit silly, but didn't care enough to complain (and, of course, I like making things more testable). However, I've noticed we've now greatly increased what gets executed with the inclusion of functions.php and every theme template file. In the case of including functions.php, we're running each function twice (via is_child_theme(), too). These functions are definitely not as cheap as a constant — they inclusively run three filters and a get_option() call (five if multiple theme roots are in play).

I have not yet run any kind of profiling on this.

Eliminating TEMPLATEPATH and STYLESHEETPATH was designed to make things more testable, I get that. But that doesn't apply to wp-settings.php, for example. This also exposes the ability to tinker with locate_template() by filtering the theme directory itself, which seems pretty scary. And, the changes in locate_template() mean these functions are called in a loop, rather than outside of it, especially problematic because it could be called up to four times per iteration even though the values might be the same (in the case of no child theme).

I just want to make sure we know what we're getting into, and also optimize for the fact that we're now dealing with dynamic code so we need to be more mindful of how we're writing our control structures.

#32 @helen
9 years ago

  • Keywords needs-patch added; has-patch removed

#33 follow-up: @nacin
9 years ago

  • Keywords revert added

Suggest we revert [28563] for now.

#34 in reply to: ↑ 33 @DrewAPicture
9 years ago

Replying to nacin:

Suggest we revert [28563] for now.

+1. Good points all in comment:31. "I just want to make sure we know what we're getting into ...".

#36 @nacin
9 years ago

  • Keywords revert removed
  • Milestone changed from 4.0 to Future Release

Happy to see this happen at some point, but it's not a priority and isn't a straight replacement due to the cost differences in these functions.

#37 @chriscct7
8 years ago

Re: comment:31 It'd be interesting to see the difference in real cost could be benchmarked for comparison

#38 @dd32
7 years ago

#37443 was marked as a duplicate.

#39 @hkchakladar
5 years ago

Are those constants deprecated? I can't see deprecated tag in WP-4.9.1

#40 follow-up: @sebastienserre
12 months ago

Hello,

I think this ticket should be consider again as logs are full of

PHP Warning:  Use of undefined constant TEMPLATEPATH - assumed 'TEMPLATEPATH' (this will throw an Error in a future version of PHP) in /var/www/XXX/prod/wp-includes/theme.php on line 158
Warning: Use of undefined constant TEMPLATEPATH - assumed 'TEMPLATEPATH' (this will throw an Error in a future version of PHP) in /var/www/XXX/prod/wp-includes/theme.php on line 158
PHP Warning:  Use of undefined constant STYLESHEETPATH - assumed 'STYLESHEETPATH' (this will throw an Error in a future version of PHP) in /var/www/XXX/prod/wp-includes/theme.php on line 158
Warning: Use of undefined constant STYLESHEETPATH - assumed 'STYLESHEETPATH' (this will throw an Error in a future version of PHP) in /var/www/XXX/prod/wp-includes/theme.php on line 158

We should maybe just check if constant are defined ?

#41 in reply to: ↑ 40 @SergeyBiryukov
12 months ago

Replying to sebastienserre:

We should maybe just check if constant are defined ?

They are defined via wp_templating_constants() early in wp-settings.php, so it seems like the only way to get these warnings is to call is_child_theme() too early: directly on file inclusion or on one of the following actions:

  • plugins_loaded
  • sanitize_comment_cookies
  • setup_theme

I suppose is_child_theme() could check if TEMPLATEPATH and STYLESHEETPATH are defined, and return false otherwise, but that might not be the expected result if a child theme is indeed in use, it's just that the check is performed too early. Perhaps a _doing_it_wrong() message would be a better option, similar to the one we have for conditional tags:

if ( ! isset( $wp_query ) ) {
	_doing_it_wrong( __FUNCTION__, __( 'Conditional query tags do not work before the query is run. Before then, they always return false.' ), '3.1.0' );
	return false;
}
Note: See TracTickets for help on using tickets.