Twenty Sixteen as default theme

[SergeyBiryukov]

Per previous Slack discussions, Twenty Sixteen will not be merged to SVN, but should be a default theme for new installs:

​https://wordpress.slack.com/archives/core/p1443650940006491

matt: what's the advantage of having 2016 in core at all?
matt: can't we just have the bundling be a packaging thing, vs a monolithic SVN thing

otto42: like, code in the upgrade process to auto-install the theme if its not there?
otto42: from themes dir

matt: don't even need to, people who want it can get it from the directory
matt: it's really more for brand new users doing one-clicks or downloads

​https://wordpress.slack.com/archives/core/p1444259632001480

ocean90: If we ship Twenty Sixteen with with core I'm fine with adding the people to the credits file. But we should test this a bit since there are some tests re WP_DEFAULT_THEME.

dd32: We’re going to have some issues with the WP_DEFAULT_THEME constant with Twenty Sixteen NOT as part of svn, and that’s expected because we can’t expect it’s installed on all sites (Especially ones which have just upgraded). What this effectively means is we have to stop using the constant.. But the constant exists because of bad hosts we couldn’t rely upon a non-constant method..

dd32: What we really need to do is to ship-with/have-in-svn one theme that’s always there (WP_DEFAULT_THEME) and have the default theme for new installs not based on WP_DEFAULT_THEME but rather based on a hard-coded ‘twentysixteen’ in the setup options.. That then has implications for Multisite though.. where WP_DEFAULT_THEME might be custom defined to something else.

Previously, all bundled themes were merged into SVN, so we should figure out the process for Twenty Sixteen, both in terms of how this is going to work for SVN and built packages, and giving credit to the theme contributors.

For reference, ​here's the list of things that needed to change when new default themes were merged in the past.

#21931 and #28645 might also be relevant.

[SergeyBiryukov]

In 35203:

Twenty Sixteen: Update Gruntfile.js to ignore the html5 shiv for JSHint.

See #34306, #34315.

[helen]

A note that one of the commits here should prop all of the contributors to Twenty Sixteen, preferably one that sets it to be the default.

[jorbin]

In 35208:

Add Twenty Sixteen to travis builds

This allows the automated tests to include Twenty Sixteen.

Fixes #34315. See #34306.

[dd32]

34306.diff​ is my attempt at setting the new default for new sites where the twentysixteen theme exists on the system.

• It keeps WP_DEFAULT_THEME at twentyfifteen as that's the latest theme we can rely upon being on the system.
• It avoids hitting the filesystem to determine WP_DEFAULT_THEME

The alternative is to alter WP_DEFAULT_THEME during the build process, if twentysixteen exists during build, set as default, else twentyfifteen. This would work for new installs, but fail on upgrades (As twentysixteen won't get installed during update, but may exist within the package, or when the package was built).

I don't know the answer here.

[nacin]

I have thought about this for a while now, and even nearly drifted off to sleep the other night trying to rack my brain on how to address this (@helen can attest). I have discovered a truly marvelous solution of this, which this margin is too narrow to contain.

No, I'm kidding. I think I got it. 34306.2.diff​ reworks how we use WP_DEFAULT_THEME. [Note: While @dd32 and I have studied the patch and its approach, it is untested.] WP_DEFAULT_THEME is typically used for falling back to or setting a default theme. The key is it needs to exist. WordPress until now has always assumed it existed, which was already an imperfect assumption, as the newest default theme could have been deleted after being installed.

So normally what we'd want to do here is one of two things:

1. Set WP_DEFAULT_THEME by default to false, then when we need to, just-in-time check to see what is installed. This doesn't work because WP_DEFAULT_THEME has always been assumed by both core — and thus presumably plugins — to be the name of a theme. We shouldn't break that assumption.
2. Set WP_DEFAULT_THEME by default to the latest default theme that is installed. This doesn't work because we'd need to hit the filesystem to determine this, and we definitely don't want to hit the filesystem in this situation.

So instead, we must do a third option:

• Set WP_DEFAULT_THEME to the latest default theme. In this case, it would be twentysixteen.
• However, it may not always exist — it could be deleted after a new install, it could be not installed during upgrade, it could be set to an invalid identifier, etc.
• Thus, every time we want to use it, we must verify its existence, and if it does not exist, then we must fall back to the latest core default theme that is installed. This could very easily be Twenty Fifteen, or it could be Twenty Ten, or it could be Kubrick.
• But, if none of them are installed, well, we're in an interesting situation. What do we do? In the case of validate_current_theme(), we should do nothing — we don't have a valid theme to switch to, so we'll have to stay with our invalid theme. In the case of populate_options() (for an initial blog), we can simply instantiate the site with WP_DEFAULT_THEME — which is what we already did previously when the theme didn't exist (we didn't check). This would only really come into play in multisite. cc @jeremyfelt. I think it's past the point where we add UI to set a network default theme. Before then, should we possibly choose a theme that is "allowed" for the network, since we can presume those exist?
• Finally, WP_DEFAULT_THEME is used in some incredible (and incredibly old) code that takes a pre-1.5 template directory, wp-layout.css file, etc., and turns it into a real WordPress theme. It'll actually continue to work (yes, I thought this out) since someone updating from 1.2 or earlier (presumably one of us trying to be funny) will need to download the zip and copy over the core files, which includes moving the new wp-content directory into place.

Additionally, the patch does *not* install Twenty Sixteen on upgrade. It still allows this to happen by listening to an explicit define of CORE_UPGRADE_SKIP_NEW_BUNDLED as false. I think we could probably wrap all default themes in that check, otherwise if you're upgrading from 4.0, you will get Twenty Fifteen but not Twenty Sixteen.

Again, note, patch not tested. Also, as @helen indicates above, this should be done in two commits, leaving the WP_DEFAULT_THEME change to twentysixteen for the second commit, and propping everyone who contributed. It's not that we couldn't include them all in the credits either way, it's that it's still cool to see your name here.

[nacin]

Slightly different handling of installing themes on upgrade.

[jeremyfelt]

I think the CORE_UPGRADE_SKIP_NEW_BUNDLED handling in 34306.3.diff​ makes sense. I've updated that patch in 34306.4.diff​ with a syntax error fix - : vs ;.

Replying to nacin:

In the case of populate_options() (for an initial blog), we can simply instantiate the site with WP_DEFAULT_THEME — which is what we already did previously when the theme didn't exist (we didn't check). This would only really come into play in multisite.

The change in behavior here is that previously, if the theme defined in WP_DEFAULT_THEME was not available, it would still be set as the theme for a new site, which would then effectively not have a theme. Now, if WP_DEFAULT_THEME is not available, and a core theme exists, that will automatically be activated. I don't think that's a bad change. Any network that wants to restrict new sites from using core themes should make sure their default theme exists. :)

I think it's past the point where we add UI to set a network default theme. Before then, should we possibly choose a theme that is "allowed" for the network, since we can presume those exist?

Agreed. We should look at adding this in the near future. I don't think we should rush into choosing an "allowed" theme as there are often going to be many for a single network. Do you take the first/last/random and activate that? I'd rather leave it for the network admin to decide until we have a better UI in place.

I haven't tested any updates using the patch, but I have gone through the process of creating a new network and sites on that network a few times and things seem stable/sane.

[helen]

Props list for the WP_DEFAULT_THEME change: ​https://github.com/WordPress/twentysixteen/blob/master/CONTRIBUTORS.md

[ocean90]

Replying to helen:

Props list for the WP_DEFAULT_THEME change: ​https://github.com/WordPress/twentysixteen/blob/master/CONTRIBUTORS.md

Please convert to w.org users first (if not already done.)

[dd32]

The change in behavior here is that previously, if the theme defined in WP_DEFAULT_THEME was not available, it would still be set as the theme for a new site, which would then effectively not have a theme.

I'm really okay with this change in behaviour. The current behaviour is just buggy and not useful.
Yes, it does mean someone may unexpectantly end up with a default theme activated when they create a site and specify a theme that doesn't exist, but the alternative of a site which doesn't have a valid theme associated with it seems worse.

[kraftbj]

Replying to ocean90:

Replying to helen:

Props list for the WP_DEFAULT_THEME change: ​https://github.com/WordPress/twentysixteen/blob/master/CONTRIBUTORS.md

Please convert to w.org users first (if not already done.)

All except drew look good—The drew user is 10 years old with no activity in the profiles.wordpress.org stream since). I assume it should be @DrewAPicture.

Otherwise, checked them all against profiles.wordpress.org and make logical sense.

[dd32]

Testing results with 34306.4.diff​

• Defining WP_DEFAULT_THEME to something that doesn't exist: WordPress selected the first core theme that exists. Pass
• Defining WP_DEFAULT_THEME to something that exists: WordPress does what it does best, just work. Pass
• Defining WP_DEFAULT_THEME as twentysixteen, activating that theme, and removing it causing WSOD: validate_current_theme() says it's a valid theme, assumes that WP_DEFAULT_THEME always exists. FAIL, Fixed
• Defining WP_DEFAULT_THEME as invalid-theme, removing the current active theme: validate_current_theme() says the theme is invalid, Switches to the first default theme as invalid-theme wasn't installed. Pass
• Creating a Network with WP_DEFAULT_THEME as invalid-theme: Fails to whitelist first default theme. Uses WP_Theme object instead of WP_Theme->get_stylesheet(): FAIL, Fixed
• Creating a new site with WP_DEFAULT_THEME as invalid-theme with first default theme whitelisted: Site created with the first default theme. Pass
• Creating a new site with WP_DEFAULT_THEME as invalid-theme with first default theme not whitelisted: Site created with the first default theme, not marked "enabled" for site, Unable to test further. Assumed OK.
• Upgrading a site: Default Themes not installed (Not even older default themes) Pass
• Upgrading a site with define( 'CORE_UPGRADE_SKIP_NEW_BUNDLED', false ) - All default themes installed Pass

The failures above were fixed as i went along, 34306.5.diff​ being the result.

Fixes:

1. validate_current_theme() - Removed the checking for WP_DEFAULT_THEME from the Invalid/Valid if branches. This was no longer needed now that the function would conditionally set the default theme.
2. schema.php - Used WP_Theme->get_stylesheet() instead of the object.

Anyone see a test case which I didn't test above?

[wonderboymusic]

In 35738:

Upgrade: New themes are not automatically installed on upgrade. This can still be explicitly asked for by defining CORE_UPGRADE_SKIP_NEW_BUNDLED as false.

In populate_options(), if the theme specified by WP_DEFAULT_THEME doesn't exist, fall back to the latest core default theme. If we can't find a core default theme, WP_DEFAULT_THEME is the best we can do.

Props nacin, jeremyfelt, dd32.
See #34306.

[helen]

In 35739:

Set Twenty Sixteen as the default theme.

With thanks to all those who contributed.

props iamtakashi, karmatosed, iandstewart, dd32, mor10, grapplerulrich, davidakennedy, frank-klein, tywayne, wenthemes, monika, metodiew, nhuja, headonfire, Chrisdc1, philiparthurmoore, karpstrucking, cais, mt8.biz, fjarrett, sdavis2702, SergeyBiryukov, eduardozulian, webdevmattcrom, ehtis, peterwilsoncc, tfrommen, fsylum, wonderboymusic, ocean90, obenland, cainm, mrahmadawais, drewapicture, trenzterra, tevko, kraftbj, walbo, nacin.
fixes #34306.

[helen]

I used the ​contributors doc from the repo for the props list - if anybody was left off or the username does not match your .org username, please leave a comment and we'll get you added to the credits API, with apologies.

[nacin]

@dd32:

• validate_current_theme() - Removed the checking for WP_DEFAULT_THEME from the Invalid/Valid if branches. This was no longer needed now that the function would conditionally set the default theme.

Just writing to say I concur with this. Keeping the check in my patch was deliberate, primarily to track existing behavior and to also avoid the filesystem hit when you were using the default theme. That said, validate_current_theme() should actually validate the theme. And while it's in wp-includes, it's only used in the customizer and in the admin. And in both cases, we're hitting the filesystem anyway, both with file_exists() and actually reading style.css via WP_Theme, which is what we're doing here.