WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#27884 closed enhancement (fixed)

Unit testing Multisite's bootstrap

Reported by: jeremyfelt Owned by: nacin
Milestone: 4.0 Priority: normal
Severity: normal Version:
Component: Bootstrap/Load Keywords: has-patch
Focuses: multisite Cc:

Description

This ticket is prodded by the cause of #27866.

We were able to add a few tests for the site and network finding process in #27003 during 3.8 development, but we're still limited by the structure of ms-settings.php pretty severely.

We should be able to test how $_SERVER requests turn into expected $current_site and $current_blog globals.

In an ideal world, we could also harness something in our tests to change the DOMAIN_CURRENT_SITE and PATH_CURRENT_SITE constants on every test to test various situations. If nothing else, we can initiate phpunit with different values.

Attachments (4)

unit-hack.diff (1.5 KB) - added by jeremyfelt 6 years ago.
27884.diff (11.9 KB) - added by jeremyfelt 6 years ago.
27884.2.diff (7.8 KB) - added by jeremyfelt 6 years ago.
27884.3.diff (1023 bytes) - added by jeremyfelt 6 years ago.

Download all attachments as: .zip

Change History (23)

@jeremyfelt
6 years ago

#1 @jeremyfelt
6 years ago

So. To start things off at the bottom.

If we move a few things from ms-settings.php to wp-settings.php and remove the need for ms-settings to call any external files, we can technically include ms-settings.php at any time. This could mean unsetting $current_site and $current_blog, changing the $_SERVER request, and then reloading that part of the bootstrap in a unit test to see if we get expected results.

unit-hack.diff does this. ;-/

#2 @nacin
6 years ago

I don't actually hate this. Nice work. Good to start somewhere.

@jeremyfelt
6 years ago

#3 @jeremyfelt
6 years ago

  • Milestone changed from Awaiting Review to Future Release

Ok. 27884.diff goes all in.

  • Introduces wp_network_is_defined(), which applies a network_is_defined filter so that we can specify whether to use DOMAIN_CURRENT_SITE, etc...
  • Introduces wp_get_defined_network(), which applies a get_defined_network filter so that we no longer need to rely on constants to predefine the network being loaded.
  • Moves the include of ms-load.php, ms-default-constants.php, and sunrise.php to wp-settings.php so that we can include ms-settings.php repeatedly while testing.
  • Removes the definition of network constants in the test bootstrap.
  • Adds basic multisite bootstrap tests for some easy scenarios.
  • Makes use of the new filters while testing so that we can modify the load "constants".

This feels nicer than I expected.

No guarantee on the full MS tests passing, there seems to be a failure of a different type with terms right now. If you run with just the 27884 group, it works fine.

#4 @DrewAPicture
6 years ago

  • Keywords needs-docs added

Hook docs.

#5 @jeremyfelt
6 years ago

Related #24173 - for testing multisite installed in a subdirectory.

This ticket was mentioned in IRC in #wordpress-dev by jeremyfelt. View the logs.


6 years ago

@jeremyfelt
6 years ago

#7 @jeremyfelt
6 years ago

27884.2.diff pulls back a bit on the previous patch.

  • Move all uses of require() to wp-settings.php and leave the include_once and ms_subdomain_constants in ms-settings as these can be called multiple times.
  • Add tests for multisite bootstrap
  • Remove use of site specific multisite constants in the tests.

#8 @jeremyfelt
6 years ago

  • Keywords has-patch added; needs-docs removed
  • Milestone changed from Future Release to 4.0

#9 @nacin
6 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 28910:

Add initial unit tests for multisite's bootstrap.

props jeremyfelt.
fixes #27884.

#10 follow-up: @danielbachhuber
6 years ago

fyi - this broke WP-CLI: https://github.com/wp-cli/wp-cli/pull/1267

Not really your fault though.

#11 in reply to: ↑ 10 @SergeyBiryukov
6 years ago

Replying to danielbachhuber:

fyi - this broke WP-CLI: https://github.com/wp-cli/wp-cli/pull/1267

Related: #28696.

@jeremyfelt
6 years ago

#12 @jeremyfelt
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

As @danielbachhuber mentioned, moving the require() statements out of ms-settings.php and wp-settings.php caused a break with WP-CLI, which closely mirrors the load process here. If the same change was made in WP-CLI, this would break back-compat.

It appears that we can instead use require_once() for ms-load and ms-default-constants and leave this in ms-settings.php without causing issue.

27884.3.diff makes this change. It's likely worth reverting [28910] for the time being to make sure this is the right move.

#13 @SergeyBiryukov
6 years ago

In 28934:

Move ms-load.php and ms-default-constants.php inclusion back to ms-settings.php to avoid breaking WP-CLI.

Use require_once() to allow for ms-settings.php to be included multiple times while testing.

props jeremyfelt.
see #27884.

#14 @jeremyfelt
6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

I think we're back to fixed in [28934]

#15 @tellyworth
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm getting a PHP warning that I think was created here:

PHP Warning:  Uncaught exception 'PHPUnit_Framework_Error_Notice' with message 'Undefined variable: wpdb' in wordpress-develop/src/wp-includes/ms-settings.php:76

The ! is_subdomain_install() block in ms-settings.php refers to $wpdb which is not in scope when called from _setup_host_request().

Adding $wpdb to the globals in _setup_host_request() fixes it.

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


6 years ago

#17 @wonderboymusic
6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 29552:

Ensure that $wpdb is imported in Tests_MS::_setup_host_request.

Props tellyworth.
Fixes #27884.

#18 @helen
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Getting a failure:

1) Tests_MS::test_multisite_bootstrap
Failed to assert that define() triggered a deprecated notice

/srv/www/wp-core/develop/tests/phpunit/includes/testcase.php:168
/srv/www/wp-core/develop/tests/phpunit/includes/testcase.php:47
/srv/www/wp-core/develop/tests/phpunit/tests/ms.php:24

#19 @helen
6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Actually, going to leave this as fixed for now.

Note: See TracTickets for help on using tickets.