WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 2 years ago

#10067 closed enhancement (duplicate)

add theme_basename(), and make sure it plays well with get_theme_root()

Reported by: bforchhammer Owned by:
Milestone: Priority: low
Severity: minor Version: 2.8
Component: Themes Keywords: needs-patch
Focuses: Cc:

Description

Stumbled on it while invastigatin #10011... theme_basename() loads the path to the theme directory using get_theme_root() but does not actually use it to replace the given path.

Don't know if that's on purpose or not but it should be fixed or at least cleaned up either way I think ;-)

Attachments (3)

10067.patch (672 bytes) - added by bforchhammer 5 years ago.
10067-without-theme-folder.patch (680 bytes) - added by bforchhammer 5 years ago.
returns path relative to "theme" folder
10067-with-theme-folder.patch (672 bytes) - added by bforchhammer 5 years ago.
returns path relative to themes folder

Download all attachments as: .zip

Change History (20)

bforchhammer5 years ago

comment:1 follow-up: dd325 years ago

looks good to me, Except you've used the wrong delim in the preg_quote call.

comment:2 follow-up: filosofo5 years ago

The regexp in the patch does not match the subdirectory of the themes/ directory, as does the original line. I think it should be:

$file = preg_replace('#^'. preg_quote($theme_dir, '#') .'/.*?/#','',$file); // get relative path from theme dir

comment:3 in reply to: ↑ 1 bforchhammer5 years ago

Replying to dd32:

looks good to me, Except you've used the wrong delim in the preg_quote call.

Oops my bad, you're right of course; copy&paste is never a good idea.. ;-)

comment:4 in reply to: ↑ 2 bforchhammer5 years ago

Replying to filosofo:

The regexp in the patch does not match the subdirectory of the themes/ directory, as does the original line. I think it should be:

$file = preg_replace('#^'. preg_quote($theme_dir, '#') .'/.*?/#','',$file); // get relative path from theme dir

Shouldn't the file name include the name of the theme folder? At least that's how plugin_basename() works; it includes the folder name of the plugin.

comment:5 follow-up: dd325 years ago

Shouldn't the file name include the name of the theme folder? At least that's how plugin_basename() works; it includes the folder name of the plugin.

I'd agree with you, Except for the fact it was done differently for a reason.

My guess is, You dont have mutliple themes active at once, like plugins, So all basenames will be relative to the current themes folder, not the themes directory.

Add a /[^/]*?/? after what you've added i think

comment:6 Denis-de-Bernardy5 years ago

  • Milestone changed from Unassigned to 2.9

comment:7 in reply to: ↑ 5 bforchhammer5 years ago

Replying to dd32:

I'd agree with you, Except for the fact it was done differently for a reason.

... which would be? :-)

So if the function is called with the filepath to a themes' functions.php you'd get "functions.php" as a result instead of "mythemename/functions.php". If we'd use it as suggested in #10011 then the result of the function would be used as an action key; I think the keyname including mythemename would be better in that case because it's more unique.

Of course perhaps it's the use of the function that would need to be different, but as far as I can see theme_basename() is also not actually being used anywhere else at the moment...

bforchhammer5 years ago

returns path relative to "theme" folder

bforchhammer5 years ago

returns path relative to themes folder

comment:8 dd325 years ago

Pretty sure theme_basename is used somewhere.. it was added for a reason.. No idea where tho :)

... which would be? :-)

The next paragraph i posted. The part about theme files only needing to be relative to the themes directory..

comment:9 follow-up: bforchhammer5 years ago

Right, it was used when the function was added in c10835 but then the function call was reverted to the use of basename() in c10951 with the commit message "Page templates should be relative to the theme ir, not the themes dir".

I vote for "with theme folder". And if only to prevent further confusion ;-)

comment:10 in reply to: ↑ 9 filosofo5 years ago

Replying to bforchhammer:

I vote for "with theme folder". And if only to prevent further confusion ;-)

That does make the most sense, since it's parallel with plugin_basename.

However, what's the point of this function if it's not used by core? Plugins can be placed directly in the plugins directory or in a sub-directory, so plugin_basename helps the plugin determine which.

In contrast, themes must be in their own sub-directory of themes. So if a theme wants its current directory name, it can get it much more efficiently with basename(dirname(__FILE__))

comment:11 Denis-de-Bernardy5 years ago

  • Milestone changed from 2.9 to 2.8

cross-referencing...

  • #4131 is where/why the function was introduced
  • #10011 (by the same reporter) highlights issues with the same function

Suggesting we get this fixed in 2.8, too, since the function was introduced in it.

comment:12 westi5 years ago

  • Milestone changed from 2.8 to Future Release

Ok I see no point in fixing this function for 2.8 as the door is pretty much close.

I am going to remove that function from 2.8 so we don't have a incorrect function to support - it is marked as private in the phpdoc so no one should be using it.

We can revisit this function in fixing the kind of issue seen in #10011 later

comment:13 westi5 years ago

(In [11545]) Remove theme_basename() for now as the changeset that introduced it was reverted and nothing uses it. See #10067.

comment:14 Denis-de-Bernardy5 years ago

  • Priority changed from normal to low
  • Severity changed from normal to minor
  • Summary changed from theme_basename() not using get_theme_root() to add theme_basename(), and make sure it plays well with get_theme_root()
  • Type changed from defect (bug) to enhancement

comment:15 Denis-de-Bernardy5 years ago

  • Keywords needs-patch added; has-patch removed

comment:16 Denis-de-Bernardy5 years ago

  • Keywords 2nd-opinion removed

comment:17 nacin2 years ago

  • Milestone Future Release deleted
  • Resolution set to duplicate
  • Status changed from new to closed

No longer necessary with WP_Theme, #20103.

Note: See TracTickets for help on using tickets.