WordPress.org

Make WordPress Core

Opened 6 weeks ago

Last modified 6 weeks ago

#43728 new enhancement

Implements site_enable_theme and site_disable_theme functions in WP_Theme class

Reported by: Mista-Flo Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch needs-unit-tests
Focuses: multisite Cc:

Description

In Multisite,

Themes can also be enabled per site, via the Edit Site > Themes area. They are stored in an allowedthemes option (see https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/class-wp-theme.php#L1414 for more). When you set a theme for a site, you wouldn't expect it to suddenly work for every network - it only implies you want to have it for the one site.

I think we need to implement methods WP_Theme::site_enable_theme( $stylesheets, $site_id ) and WP_Theme::site_disable_theme( $stylesheets, $site_id ).

We need this enhancement in this ticket : #39318

Attachments (2)

43728.1.patch (2.1 KB) - added by Mista-Flo 6 weeks ago.
Implementation of first Idea : In WP Theme class
43728.2.patch (2.2 KB) - added by Mista-Flo 6 weeks ago.
Fixes after review

Download all attachments as: .zip

Change History (9)

#1 follow-up: @swissspidy
6 weeks ago

Thinking out loud here:

What about something like WP_Site::enable_theme( WP_Theme $theme )? and WP_Network::enable_theme( WP_Theme $theme ) (or WP_Network::enable_theme( string $stylesheet )) instead of WP_Theme:: network_disable_theme( $stylesheets ).

Then you could just do get_network()->enable_theme(…) or get_site()->enable_theme(…).

The existing WP_Theme methods could be simple wrappers about the new ones.

In other words, I think it's more of an action on the site/network instead of the theme.

#2 @Mista-Flo
6 weeks ago

It makes sense I think, what do you think @flixos90 ?

@Mista-Flo
6 weeks ago

Implementation of first Idea : In WP Theme class

#3 @Mista-Flo
6 weeks ago

  • Keywords has-patch added

#4 in reply to: ↑ 1 @flixos90
6 weeks ago

  • Keywords has-patch removed

Replying to swissspidy:

What about something like WP_Site::enable_theme( WP_Theme $theme )? and WP_Network::enable_theme( WP_Theme $theme ) (or WP_Network::enable_theme( string $stylesheet )) instead of WP_Theme:: network_disable_theme( $stylesheets ).

That's a good idea to consider. I'm not sure we wanna go that route yet though, as we'd need to discuss whether WP_Site and WP_Network should be more than representations of their core data. The common pattern currently is to have functions that access those objects as necessary, not methods on those objects that then do logic. If we went this approach, we'd also open up possible WP_Network::*_option() methods and many more, which eventually would cause the classes to be way too large.

I can see where your idea comes from, and it's kind of a shame that WP_Theme includes those methods because they only really make sense in multisite - if it wasn't for legacy, I'd go with a factory that would use another class in multisite that has those methods, but in single-site they wouldn't be available.

I suggest for now, mostly for consistency, we go with methods on WP_Theme. Alternative patterns involving WP_Site and WP_Network could still be implemented at a later stage.

#5 @flixos90
6 weeks ago

  • Keywords has-patch added

#6 @flixos90
6 weeks ago

  • Keywords needs-unit-tests added

@Mista-Flo

43728.1.patch looks good, just a few improvements:

  • The paramters should be switched: $stylesheets should be first, the optional $site_id parameter second, as it falls back to the current site ID if not given.
  • The parameter doc-block entries should be properly aligned with spaces.
  • At the beginning of the methods, $site_id must be cast to an integer, to make sure the strict comparisons afterwards work correctly.

We'll also need a few tests to verify the correct behavior.

@Mista-Flo
6 weeks ago

Fixes after review

#7 @Mista-Flo
6 weeks ago

Thanks for the review @flixos90

I have updated the patch.

For the last thing, I have never wrote a unit test, So I'm gonna need some help on this.

Note: See TracTickets for help on using tickets.