Make WordPress Core

Opened 4 years ago

Last modified 18 months ago

#50018 new enhancement

Site Health: is_writable() check when there are more than 1 theme directories

Reported by: pbiron's profile pbiron Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords:
Focuses: Cc:

Description

While working on the patch to #48199, I discovered the existence of register_theme_directory(). Using register_theme_directory() it seems to be possible to have a setup such as:

  • WP_CONTENT_DIR/parent-themes/some-parent-theme
  • WP_CONTENT_DIR/child-themes/current-theme

where current-theme is a child of some-parent-theme. In such a setup, the current behavior (and that in the patch for #48199) actually checks whether WP_CONTENT_DIR/parent-themes is writable.

2 questions:

  1. should the is_writable() actually be checking whether get_theme_root( get_stylesheet() ) is writable?
  2. should there be additional checks whether all registered theme directories are writable?

Change History (6)

#1 @Clorith
23 months ago

  • Milestone changed from Awaiting Review to 6.1

Hmm, yeah should probably be done. The intention of the check is to see if theme installations/updates can happen, for installations the default location would be used, but updates would use the theme root location, so makes sense to loop over them and check them all.

Great catch!

#2 @Clorith
19 months ago

  • Keywords needs-patch added

Is this something you'd like to work on @pbiron?

I'm thinking we need to actually grab the $wp_theme_directories global variable for this one, and check if it's got more than one entry; loop over them and check if they are writable, but only if they are within ABSPATH to avoid hitting the same kind of scenario as the symlink issue you noted in the previous ticket. (This because it's possible to provide an absolute path, alongside the ability to register a relative path from wp-content)

#3 @pbiron
19 months ago

sure. But I don't think I'll have the time before 6.1 beta, but certainly for 6.2 (or potentially a 6.1.x).

This ticket was mentioned in PR #3245 on WordPress/wordpress-develop by thijsoo.


18 months ago
#4

  • Keywords has-patch added; needs-patch removed

I made sure all themes in the wp_theme_directories global are checked for wp_is_writable
Trac ticket: https://core.trac.wordpress.org/ticket/50018

Clorith commented on PR #3245:


18 months ago
#5

Thank you for the patch @thijsoo !

In addition to the suggested variable naming from @costdev, there's a few things we need to keep track of, the current proposed solution would set the value of $is_writable_template_directory to the status of the very last directory being checked, so instead it may be an idea to instead show each available directory, and if they are writable or not, as the value (this can be passed as an array, some pseudo-code for you below to demonstrate)

{{{php
$is_writable_value = [];
$is_writable_debug = [];
foreach ( $theme_directories as $theme_directory ) {

$is_writable_value[] = sprintf(

'%s: %s',
$theme_directory,
( wp_is_writable( $theme_directory ) ? ( 'Writable' ) : ( 'Not writable' ) )

);

$is_writable_debug[] = sprintf(

'%s: %s',
$theme_directory,
( wp_is_writable( $theme_directory ) ? 'writable' : 'not writable' )

);

}
}}}

(Note that it needs two variables, as the debug value is never translated, for consistency between languages in debug data.)

The label for "The themes directory" should also be updated to use the `_n()` translation function, so that it can account for if there is only one, or multiple theme directories available.

#6 @Clorith
18 months ago

  • Keywords has-patch removed
  • Milestone changed from 6.1 to Future Release
  • Version set to 5.2

As beta-1 is planned for tomorrow, and the patch still needs some refinement, I'm moving it out of the milestone for now (but will keep watching it, so feel free to improve on the patch at your own time!)

Note: See TracTickets for help on using tickets.