add test to see if sunrise === on in ms-settings

[sboisvert]

We needed to disable sunrise for certain requests.

We thought about declaring sunrise before including wp-load, but setting it to 'off'. Then having a conditial in the wp-config that only defines it to on if it is not defined and then in ms-settings to a additional check to see if sunrise is defined and equal to on. (the patch I am adding does not have the wp-config if !defined() as wp-config doesn't get updated, but if someone feels it is helpful I can add it)

Others could of changed the default defined value of sunrise and this could break bc. I am therefore not sure if the limited use case warrants it. But I thought I would suggest it.

[scribu]

We thought about declaring sunrise before including wp-load, but setting it to 'off'. Then having a conditial in the wp-config that only defines it to on if it is not defined and then in ms-settings to a additional check to see if sunrise is defined and equal to on.

That sounds extremely convoluted. Are you sure the stuff you're trying to disable belongs in sunrise.php in the first place?

[sboisvert]

That's quite possible.

It's to disable domain mapping and url rewriting for auto resized image downloads so that all thumbnails go thru the main site.

[johnbillion]

The attached patch would break a site of mine which uses define('SUNRISE',true). We should patch it in the same manner as #20636.

[johnbillion]

Patch updated so any non-empty value for SUNRISE is accepted.

[nacin]

The 'SUNRISE' constant is one of those constants that, as long as it is defined in any way, triggers is_multisite() to return true. I don't find this to be a deal-breaker as to whether we assert truthiness before deciding whether to load sunrise.php, but it's nice to be consistent and at least understand what is going on.

The good thing is, SUNRISE never got the ridiculous "off" and "on" treatment given to VHOST. Setting it to "off" did not appear to have an effect in MU. So this change should be backwards compatible in a sane way, and seems sensible enough.

But I must ask, if you're going to disable sunrise for certain requests, why not just not define it?

[sboisvert]

I think what you are saying is that I should of done something like

if ( !defined('SUNRISE') && !defined('DONT_DEFINE_SUNRISE') )
    define( 'SUNRISE', 'on' );

in my wp-config. I really don't have a good answer for why I didn't do that....

[wpmuguru]

You can add a simple check to the beginning of sunrise.php and if some condition is met and return.

Even though there is a sunrise.php included in domain mapping sunrise.php is intended to be manually maintained. There are lots of different things that you can do in sunrise.php. If you have two plugins that both use sunrise.php you have manually create your own sunrise.php that correctly incorporates the code from both plugins.

[jeremyfelt]

20220.patch​ from @johnbillion still applies cleanly and seems sane. If SUNRISE is defined and anything non-empty, then sunrise.php is loaded.

If that defies expected back compat behavior, then we should close this ticket.

[nacin]

The one problem with 20220.patch​ is it could be fairly catastrophic for a site that is somehow defining SUNRISE as something falsey (even just an empty string), and suddenly sunrise.php doesn't load. I think I am OK with if ( defined( 'SUNRISE' ) && false !== SUNRISE ).

[nacin]

If you need to ignore SUNRISE for a particular request, then either A) don't define it, or B) if it's too late to not define it, just don't do anything in sunrise.php. WordPress doesn't skip any of its own logic when sunrise.php is included and does nothing. Ultimately, "We needed to disable sunrise for certain requests." is just not enough to justify 20220.patch​ or the like.

And that said, I'm inclined to think that even false !== SUNRISE could break things. There seems to be no inherent harm to the current approach. Wontfix.

[SergeyBiryukov]

#26678 was marked as a duplicate.

[pdclark]

Is there any room for reconsideration on this? It seems much more reasonable that wp-config.php constants work consistently than to expect any developer would ever set define( 'SUNRISE', false ); to enable sunrise!

[jeremyfelt]

Another, albeit unlikely, scenario to consider is a configuration relying on previous core behavior.

If we stopped loading sunrise on false === SUNRISE, then code in a plugin or mu-plugin that also checks is_defined( 'SUNRISE' ) would attempt to fire under the incorrect assumption that sunrise.php had been loaded.

[pdclark]

That's a great point, Jeremy.

There is the SUNRISE_LOADED constant for that purpose, but that doesn't mean the situation might not apply.

Looking at ​this code search of all plugins on wordpress.org, I count only two lines from two plugins that this *might* effect, but neither case is critical behavior — both are simply checks to display a warning:

• ​wordpress-mu-domain-mapping
• ​yet-another-multi-site-manager

The only other cases I saw in plugins either use it as a redundancy for checking multisite, or check it correctly:

• ​mailin — redundantly checking if multisite. Still works.
• ​mailchimp— same
• ​all-in-one-seo-pack — Does it right.

[dd32]

Most plugins that would make use of the constant are things that are most likely going to be custom plugins. Ultimately if we care about backwards compatibility we cannot change this at all, as silly as it may seem, so further discussion is pretty pointless.

I honestly agree with nacin when he says:

If you need to ignore SUNRISE for a particular request, then either A) don't define it, or B) if it's too late to not define it, just don't do anything in sunrise.php. WordPress doesn't skip any of its own logic when sunrise.php is included and does nothing. Ultimately, "We needed to disable sunrise for certain requests." is just not enough to justify 20220.patch​ or the like.

Define the constant when it's needed, otherwise, do nothing.. It's the act of defining a constant that we care about here, not what you define it to.
It should be pretty obvious to a developer straight away that defining it to false isn't doing what they expect.

The closest we'd get to altering the values you can define it to, is to create a new constant instead (ie. USE_SUNRISE) and only define SUNRISE for backwards compatibility if that's defined to true - but doing this provides no tangible benefit other than adding a constant and people getting confused as to why there's two (and then defining both..)

[nacin]

SUNRISE_LOADED doesn't exist in WordPress core.

This isn't about plugin compatibility, ultimately. I wish it were, I'd have less qualms about breaking this. Rather, this is about someone updating to the newest version of WordPress (a version that has possibly been running since the MU days) and suddenly finding that their sunrise.php file — which, when used, is usually extremely important — is no longer being loaded, and their site has crashed. That's the big picture. It's also a doomsday scenario, but think about how terrible this would be.

Since SUNRISE is used to detect is_multisite(), if you don't want to switch to SUBDOMAIN_INSTALL for that, you could also do this:

define( 'SUNRISE', false );

And in your sunrise.php file:

if ( false === SUNRISE ) return;

An alternative option we have is when SUNRISE is defined as false, to issue a notice saying "uh, your sunrise is still being loaded, just a heads up". But that would ingrain this functionality, unless we linked to a Codex page that explained "uh, your sunrise is still being loaded. change to true if that's what you want. if that's not what you want, add if ( false === SUNRISE ) return; to the top of sunrise.php, that way we can change this in WordPress 4.x".

This is certainly not ideal. ms-settings.php is due for some sort of overhaul eventually. This is certainly something to keep in mind.

[pdclark]

I'd certainly be in favor of a notice. While the inconsistent behavior in constants certainly sets off red flags, my main concern is usability. If developers were at least notified, I think it would save people from the situation I encountered: 24 hours of 3 engineers trying to figure out, "why the heck is everything broken?" (when sunrise was "off"). It certainly seems like a cautious and productive route to revision in 4.x. :)