WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 8 weeks ago

#31620 new defect (bug)

get_raw_theme_root does not resolve custom theme folder when is the unique

Reported by: giuseppe.mazzapica Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.1
Component: Themes Keywords: has-patch needs-testing
Focuses: Cc:

Description

get_raw_theme_root (https://developer.wordpress.org/reference/functions/get_raw_theme_root/) does this check:

if ( count($wp_theme_directories) <= 1 )
   return '/themes';

It means that if there is just one theme directory it is always assumed to be the /themes folder inside content directory.

If an user wants to register a custom theme folder via register_theme_directory() (in a MU plugin) that user is forced to keep WP_CONTENT_DIR/themes folder (even empty) otherwise the custom directory registration does not work and WP will try to load a theme from a folder that doesn't exist.

That is an edge case, but can be easily solved by picking the unique folder if there is only one:

$count = count($wp_theme_directories);

if ( 0 === $count ) {
    return '/themes';
}

if ( 1 === $count ) {
    $path = reset($wp_theme_directories);
    $real = realpath($path);
    $content = realpath(WP_CONTENT_DIR);
    return strpos($real, $content) === 0 ? substr($real, strlen($content)) : $path;
}

If there is only one folder and it is inside content directory, the code above also takes care of returning the relative path.
That, according to #17597, is necessary to obtain a correct theme url.

Attachments (2)

31620.patch (911 bytes) - added by McGuive7 4 years ago.
31620.2.patch (1.4 KB) - added by McGuive7 4 years ago.
Update patch to use wp_normalize_path() in place of realpath() to avoid potentialissues with symlinks.

Download all attachments as: .zip

Change History (16)

#1 @jeremyfelt
4 years ago

  • Component changed from Bootstrap/Load to Themes

#2 @obenland
4 years ago

  • Keywords needs-patch added

Would you want to create a patch for that?

@McGuive7
4 years ago

#3 follow-up: @McGuive7
4 years ago

  • Keywords has-patch needs-unit-tests added; needs-patch removed
  • Version set to trunk

Here's a patch that includes the code you suggested @giuseppe.mazzapica. I've modified the variable names to be slightly more semantic (e.g. $path => $theme_path) and to match existing code. Update also includes adding whitespace to match WordPress coding standards.

Tested and appears to be working in all of the following situations:

  • No registered theme directories (returns /themes)
  • Default /themes directory as only registered theme directory ( returns /themes)
  • Custom /custom-themes directory as only registered theme directory (returns /custom-themes)
  • Multiple theme registered theme directories (returns correct value depending on $stylesheet_or_template parameter

#4 in reply to: ↑ 3 @giuseppe.mazzapica
4 years ago

Replying to McGuive7:

Here's a patch that includes the code you suggested @giuseppe.mazzapica. I've modified the variable names to be slightly more semantic (e.g. $path => $theme_path) and to match existing code. Update also includes adding whitespace to match WordPress coding standards.

+1 and thanks

#5 @McGuive7
4 years ago

  • Keywords dev-feedback needs-testing added; needs-unit-tests removed

#6 follow-up: @McGuive7
4 years ago

Would appreciate a second set of eyes on this one from anyone interested.

#7 in reply to: ↑ 6 @giuseppe.mazzapica
4 years ago

Replying to McGuive7:

Would appreciate a second set of eyes on this one from anyone interested.

Sure. I asked @toscho and @f-j-kaiser (that I saw watching this ticket) to review your patch and gives some feedback. Let's see what happen.

#8 @F J Kaiser
4 years ago

  • Keywords dev-feedback removed

@McGuive7 Looks good for me – and yes, this is easy to read and understand. Both you and @giuseppe.mazzapica Good work!

#9 @giuseppe.mazzapica
4 years ago

@McGuive7 after a conversation with @toscho both of us are concerned about the use of realpath.

Our concerns regard the implications related to symlinks that may break urls because realpath expand them. (lines 637 and 638 in your patch)

Probably usage of wp_normalize_path is better because keeps the strpos check cross-OS compatible and avoid such problems, making this patch pretty a no-brainer.

For symlinks-related edge cases, there's always the filter 'theme_root_uri' that allows to change the generated url if it's wrong for any reason.

Last edited 4 years ago by giuseppe.mazzapica (previous) (diff)

@McGuive7
4 years ago

Update patch to use wp_normalize_path() in place of realpath() to avoid potentialissues with symlinks.

#10 follow-up: @McGuive7
4 years ago

Thanks for the feedback guys. Just updated the patch to include wp_normalize_path() in place of realpath(). Invaluable to get your feedback and learn something new - thanks!

One other concern, and this may be a non-issue however I'd love to get a 2nd opinion. It has to do with this line of code:

return strpos( $theme_path_normalized, $content_path_normalized ) === 0 ? substr( $theme_path_normalized, strlen( $content_path_normalized ) ) : $theme_path_normalized;

This conditional seems a bit odd to me. If it resolves to true (aka the theme path starts with the content path) then we return a relative path to the themes folder - in most cases this is /themes.

If the conditional fails, however, then we end up returning the normalized theme path (not relative URL or directory), which ends up being something like /srv/www/wordpress-develop/src/wp-content/themes.

Seems like a fault that the function could return a relative path when it's pointing to a theme directory within the content directory, but to a full path otherwise. Now, to be fair, it seems like the only way this would happen is if someone deregistered the default themes directory entirely, and then registered a new directory that is not in the content directory, but if this conditional check is going to be there at all, it seems like we may want to attempt to get the relative path of the custom directory instead of its full path, right? Thoughts?

#11 in reply to: ↑ 10 @giuseppe.mazzapica
4 years ago

Replying to McGuive7:

One other concern, and this may be a non-issue however I'd love to get a 2nd opinion. It has to do with this line of code:

return strpos( $theme_path_normalized, $content_path_normalized ) === 0 ? substr( $theme_path_normalized, strlen( $content_path_normalized ) ) : $theme_path_normalized;

[...]

The conditional is a bit odd, I know, but is required according to #17597 (that was mentioned in OP).

If you read till the end, you'll see that with [20016] has been introduced a change that (quoting @nacin in ticket:17597#comment:12)

Fix the return value of get_theme_root() when the theme root is outside of WP_CONTENT_DIR, thus making it absolute rather than the typical relative theme root.

Make get_theme_root_uri() tolerate an absolute path for a theme root. It will now make an attempt to find a corresponding URL for absolute paths as well.

#12 @McGuive7
4 years ago

Got it. Thanks for that update! This last patch should be all set then. 2nd eyes welcome. Any reason not to add the dev-feedback tag at this point? I saw you removed it previously @F J Kaiser.

#13 @obenland
4 years ago

  • Version changed from trunk to 3.1

Introduced in [15641].

#14 @toscho
4 years ago

A note on readability: can we please get rid of the overly long ternary return expression? This is not easy to read:

return strpos( $theme_path_normalized, $content_path_normalized ) === 0 ? substr( $theme_path_normalized, strlen( $content_path_normalized ) ) : $theme_path_normalized;

It should rather look like this:

if ( strpos( $theme_path_normalized, $content_path_normalized ) === 0 ) {
	$length = strlen( $content_path_normalized );
	return substr( $theme_path_normalized, $length;
}

return $theme_path_normalized;

Oh, and what happens when there are multi-byte characters in the content path? strlen() counts bytes, not characters. Will that still work? This is probably not a very common setup, but we should cover this edge case with mb_strlen() and mb_substr(). Thoughts?

Note: See TracTickets for help on using tickets.