WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#34941 closed enhancement (fixed)

Make the main bootstrap process in ms-settings.php testable

Reported by: jeremyfelt Owned by: jeremyfelt
Milestone: 4.6 Priority: normal
Severity: normal Version:
Component: Bootstrap/Load Keywords: has-patch has-unit-tests needs-testing commit
Focuses: multisite Cc:
PR Number:

Description

One of the harder things to test right now in multisite is the bootstrap behavior. We have a hobbled together process right now that sets up a new "request" and re-includes ms-settings.php before checking the status of $current_blog and $current_site. This works for many configurations, but isn't useful when catching a failed scenario that results in a redirect or ms_not_installed().

We can pull most of this code out into a new function, ms_bootstrap() or similar, that handles the domain and path and returns the possible actions. From my first pass, I've landed on true for when a site and network are found, false for when a site or network is not found, and URL string for when a request needs to be immediately redirected elsewhere.

I'm attaching an initial patch to start work on this. Once a version of it is in, we'll be able to better test a ticket like #17376.

Attachments (4)

34941.diff (28.2 KB) - added by jeremyfelt 4 years ago.
34941.2.diff (28.3 KB) - added by jeremyfelt 4 years ago.
34941.3.diff (38.0 KB) - added by jeremyfelt 3 years ago.
34941.4.diff (18.5 KB) - added by jeremyfelt 3 years ago.

Download all attachments as: .zip

Change History (34)

@jeremyfelt
4 years ago

#2 @ericlewis
4 years ago

This is cool. Maybe wp_setup_current_site_and_blog() for a more self-descriptive name?

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


4 years ago

#4 @chriscct7
4 years ago

  • Keywords needs-unit-tests added

#5 @jeremyfelt
4 years ago

  • Milestone changed from 4.5 to Future Release

Punting on this to handle it earlier in a release for more testing.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


4 years ago

#7 @jeremyfelt
4 years ago

  • Keywords 4.6-early added

@jeremyfelt
4 years ago

#8 @jeremyfelt
4 years ago

34941.2.diff is a refresh against trunk and uses wp_setup_current_site_and_blog() rather than ms_bootstrap(). I agree that's closer to what the function should be called, not sure if we're there yet.

Tests are still passing. I'll be trying to break this more over the next couple weeks. Would like some testing of this patch for sure. :)

#9 @jeremyfelt
4 years ago

  • Keywords has-unit-tests needs-testing added; needs-unit-tests 4.6-early removed
  • Milestone changed from Future Release to 4.6

#10 @jeremyfelt
3 years ago

In 37226:

Multisite: Make $current_site and $current_blog explicitly global.

See #34941.

@jeremyfelt
3 years ago

#11 @jeremyfelt
3 years ago

  • Owner set to jeremyfelt
  • Status changed from new to accepted

34941.3.diff adjusts a few things:

  • New function in ms-load.php is ms_load_current_site_and_network().
  • Try to use discouraging language for use of this outside of core, also mark as @private.
  • Add a $subdomain parameter so that we can test both subdomain and subdirectory configs with relative ease.
  • Add bootstrap tests to account for subdirectory path segments of 2 vs the default of 1. This is marked as a todo in the current tests.
  • Add bootstrap tests for subdomain configuration. :tada:

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


3 years ago

#13 @jeremyfelt
3 years ago

In 37234:

Tests: Share test fixtures in multisite bootstrap tests

  • Remove unnecessary setUp and tearDown methods.
  • Create networks and sites in wpSetupBeforeClass to share throughout.
  • Destroy networks and sites in wpTearDownAfterClass to unpollute.

See #36566, #34941.

#14 @jeremyfelt
3 years ago

In 37236:

Tests: Use a data provider to test get_network_by_path()

See #36566, #34941.

#15 @jeremyfelt
3 years ago

In 37237:

Tests: Use a data provider in get_site_by_path() tests

See #36566, #34941.

@jeremyfelt
3 years ago

#16 @jeremyfelt
3 years ago

34941.4.diff is refreshed against current trunk after the structural changes to the multisite bootstrap tests. The tests diff is a lot cleaner now. There are some more specific tests included in 34941.3.diff that are not yet part of the refresh.

This ticket was mentioned in Slack in #core by jeremyfelt. View the logs.


3 years ago

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


3 years ago

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


3 years ago

This ticket was mentioned in Slack in #core by jeremyfelt. View the logs.


3 years ago

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


3 years ago

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


3 years ago

#23 @ocean90
3 years ago

  • Keywords commit added

I've tested 34941.4.diff on w.org and haven't noticed any issues so far.

Some minor things for the patch itself:

#24 @jeremyfelt
3 years ago

In 37475:

Multisite: Wrap the main bootstrap process in a function

Introduce ms_load_current_site_and_network. This is used by core during the multisite bootstrap process to populate the $current_site and $current_blog globals based on a requested domain and path.

Return values from this function inform ms-settings.php as to whether a page view should continue, ms_not_installed() should fire, or a redirect to a new location should occur.

This was previously a procedural block in ms-settings.php. Wrapping this code and providing specific return values allows us to write tests that do not rely on the manual and repeated inclusion of ms-settings.php.

This should not be used by plugins or themes. Please.

See #34941.

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


3 years ago

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


3 years ago

#27 @jeremyfelt
3 years ago

Keeping this open for the sake of eyeballs at this point. I would like to expand tests for things like #17376, though that can always go on that ticket.

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


3 years ago

#29 @jeremyfelt
3 years ago

  • Keywords needs-dev-note added
  • Resolution set to fixed
  • Status changed from accepted to closed

Things seem good here. We can use #36566 to focus on any bootstrap tests.

#30 @ocean90
3 years ago

  • Keywords needs-dev-note removed
Note: See TracTickets for help on using tickets.