WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 8 months ago

Last modified 4 months ago

#20220 closed enhancement (wontfix)

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

Reported by: sboisvert Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.0
Component: Multisite Keywords: has-patch
Focuses: Cc:

Description

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.

Attachments (2)

ms-settings.php.patch (542 bytes) - added by sboisvert 2 years ago.
20220.patch (533 bytes) - added by johnbillion 2 years ago.

Download all attachments as: .zip

Change History (22)

sboisvert2 years ago

comment:1 scribu2 years ago

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?

comment:2 sboisvert2 years ago

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.

comment:3 johnbillion2 years ago

  • Keywords needs-patch added; has-patch removed
  • Severity changed from trivial to normal
  • Version set to 3.0

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

johnbillion2 years ago

comment:4 johnbillion2 years ago

  • Keywords has-patch added; needs-patch removed

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

comment:5 SergeyBiryukov2 years ago

  • Keywords commit 3.5-early added
  • Milestone changed from Awaiting Review to Future Release

comment:6 nacin22 months ago

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?

comment:7 sboisvert22 months ago

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....

Last edited 22 months ago by sboisvert (previous) (diff)

comment:8 wpmuguru18 months ago

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.

Last edited 18 months ago by wpmuguru (previous) (diff)

comment:9 jeremyfelt8 months ago

  • Keywords commit 3.5-early removed

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.

comment:10 jeremyfelt8 months ago

  • Milestone changed from Future Release to 3.7

comment:11 nacin8 months ago

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 ).

comment:12 nacin8 months ago

  • Milestone 3.7 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

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.

comment:13 SergeyBiryukov4 months ago

#26678 was marked as a duplicate.

comment:14 pdclark4 months ago

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!

comment:15 jeremyfelt4 months ago

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.

comment:16 pdclark4 months ago

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:

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

comment:17 dd324 months ago

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..)

comment:18 nacin4 months ago

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.

comment:19 rachelbaker4 months ago

  • Cc rachel@… added

comment:20 pdclark4 months ago

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. :)

Note: See TracTickets for help on using tickets.