WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 8 months ago

#39318 new enhancement

Assign a theme when creating a site (Multisite)

Reported by: Mista-Flo Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Networks and Sites Keywords: has-patch needs-testing
Focuses: multisite Cc:
PR Number:

Description

This is a serie of multisite focus enhancements for themes, the major one is from #13743 that add a network default theme (I used this network option in this following patch).

This ticket "Assign a theme when creating a site" was a shared idea discussed on WCUS, WC Cologne, etc.

So here a first patch that just add a select input containing the activated themes of the current network. By default, the selected theme is WP_DEFAULT_THEME constant or the new default network option for theme.

After the form is submited, we add stylesheet and template name to the $meta array, and wpmu_create_blog function makes an automatic update option with the right theme.

Attachments (6)

39318.1.patch (4.0 KB) - added by Mista-Flo 3 years ago.
First patch
Add New Site ‹ Network Admin — WordPress.png (143.2 KB) - added by Mista-Flo 3 years ago.
Screenshot with the new select field
39318.2.patch (2.1 KB) - added by Mista-Flo 2 years ago.
Better patch
39318.3.patch (1.9 KB) - added by Mista-Flo 20 months ago.
39318.4.patch (1.8 KB) - added by Mista-Flo 20 months ago.
Fix indentation
39318.5.patch (1.6 KB) - added by Mista-Flo 20 months ago.
Various fixes : Review by @flixos90

Download all attachments as: .zip

Change History (20)

#1 @Mista-Flo
3 years ago

  • Keywords has-patch needs-testing dev-feedback added

Please test this ticket with the other https://core.trac.wordpress.org/ticket/13743

@Mista-Flo
3 years ago

First patch

@Mista-Flo
3 years ago

Screenshot with the new select field

#2 @Mista-Flo
2 years ago

  • Component changed from Themes to Networks and Sites

This ticket was mentioned in Slack in #core-multisite by florian-tiar. View the logs.


2 years ago

#4 @flixos90
2 years ago

Having had a first look at the patch, it looks good so far.

We should think about whether we actually want to allow all installed themes in the dropdown, not only those enabled network-wide. It's the network administrator who creates the site, so they should have access to it, and when they choose a theme that is not network-enabled, it could automatically be enabled for the site maybe.

Those are all just thoughts, that we should think through though.

#5 @Mista-Flo
2 years ago

Thanks for your review Felix.

I guess it makes sense, I'm okay with that.

Remark : Be carreful, my patch rely on new feature added in this patch : #13743. But it needs a refresh to use the new network_get_default_theme from this patch instead of always using get_network_option function.

By the way, Tell me if we need to make a patch that is not linked to the other ticket mentioned above.

I can also avoid repeating myself two times with :

<?php
$stylesheet = $theme->get_stylesheet(); 
$template   = $theme->get_template(); 

I'll try to publish a new patch.

Last edited 2 years ago by Mista-Flo (previous) (diff)

@Mista-Flo
2 years ago

Better patch

#6 @Mista-Flo
2 years ago

The new patch use the network_get_default_theme function, list all themes in the select (even non network enabled ones), and automaticaly network enable themes if they're not like @flixos90 proposed.

#7 @flixos90
20 months ago

I just reviewed 39318.2.patch. Some notes:

  • Let's not rely on #13743 here. It's not necessary for this feature to work, we can use WP_DEFAULT_THEME for now. Once #13743 gets ready, we simply need to update the code.
  • If the theme requested doesn't exist (see line 68), nothing should change from the current behavior. I recommend you wrap all the following code that eventually updates the $meta array into a conditional that only runs if $theme->exists(). In other words: Keep $meta the public flag only in case the requested theme doesn't exist, only set stylesheet and template if it does exist. Alternatively, we might wanna wp_die() with an error, but since providing the theme is not crucial, I'm not sure we need to do that.
  • A minor suggestion: Your patch is not wrong there at all, but in line 304 and following, I think it's better readable to not use printf() and instead output HTML directly. Same for the select tag itself, it should not be echoed, but directly outputted as HTML.

#8 @Mista-Flo
20 months ago

  • Keywords dev-feedback removed

Hello @flixos90,

Thank you for your review.

I have updated my patch to fit your suggestions.

@Mista-Flo
20 months ago

@Mista-Flo
20 months ago

Fix indentation

#9 follow-up: @flixos90
20 months ago

Thanks for the update @Mista-Flo!

Just a few minor observations regarding 39318.4.patch (some of which I could have noticed earlier, sorry about that):

  • The comment on line 70 can be removed. We're not looking at WP_DEFAULT_THEME there.
  • In line 73 and 74, the equal signs should be aligned.
  • The comment on line 76 can be removed.
  • In line 78, the site should not be enabled on the network, but on the new site. Enabling it for the entire network would be unexpected.
  • In line 297, you can remove the is_array() check, the return value of wp_get_themes() should always be an array.

@Mista-Flo
20 months ago

Various fixes : Review by @flixos90

#10 in reply to: ↑ 9 ; follow-up: @Mista-Flo
20 months ago

Hi,

Thank you,

I have uploaded a new patch.

Except about the "In line 78, the site should not be enabled on the network, but on the new site. Enabling it for the entire network would be unexpected."

Are you sure you can use a theme on a site whereas its not network enabled ? I'm not sure about this.

Replying to flixos90:

Thanks for the update @Mista-Flo!

Just a few minor observations regarding 39318.4.patch (some of which I could have noticed earlier, sorry about that):

  • The comment on line 70 can be removed. We're not looking at WP_DEFAULT_THEME there.
  • In line 73 and 74, the equal signs should be aligned.
  • The comment on line 76 can be removed.
  • In line 78, the site should not be enabled on the network, but on the new site. Enabling it for the entire network would be unexpected.
  • In line 297, you can remove the is_array() check, the return value of wp_get_themes() should always be an array.

#11 in reply to: ↑ 10 ; follow-up: @flixos90
20 months ago

Replying to Mista-Flo:

Except about the "In line 78, the site should not be enabled on the network, but on the new site. Enabling it for the entire network would be unexpected."

Are you sure you can use a theme on a site whereas its not network enabled ? I'm not sure about this.

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 ). That should happen in a separate ticket though. When that is done, we can complete work on this one here. Everything else in this ticket here is ready.

#12 @swissspidy
20 months ago

That separate ticket is #43728.

#13 @Mista-Flo
9 months ago

@swissspidy @flixos90 Hey, no news here ?

#14 in reply to: ↑ 11 @swissspidy
8 months ago

  • Milestone changed from Awaiting Review to Future Release

Hey @Mista-Flo, sorry for the delay here. Multisite isn't really my field of expertise, so I leave this to the more knowledgable :-)

If you wanna help move this forward by addressing the last notes, that would be awesome.

The concern was this:

I think we need to implement methods WP_Theme::site_enable_theme( $stylesheets, $site_id ) and WP_Theme::site_disable_theme( $stylesheets, $site_id ). That should happen in a separate ticket though. When that is done, we can complete work on this one here. Everything else in this ticket here is ready.

These methods would be like \WP_Theme::network_enable_theme() and \WP_Theme::network_disable_theme, but just for a single site, not for all sites in a network.

If you want to help move this forward, I think it can also happen in this ticket though for the sake of easier development.

Note: See TracTickets for help on using tickets.