Use a non-zero exit code when requirements are not met

[jrf]

Currently, when the (updated) minimum PHP/MySQL requirements are not met, WP will exit with an error message, but will not emit an error code.

This is interpreted by automated scripts & test suites as error code 0 (= success).

In effect, this means that a unit test run for WP 5.2 on PHP 5.4 would currently show as successful, while no unit tests are being run at all.

Example output:

Your server is running PHP version 5.4.20 but WordPress 5.3-alpha-45282-src requires at least 5.6.20.Running as single site... To run multisite, use -c tests/phpunit/multisite.xml

Warning: Cannot modify header information - headers already sent by (output started at /path/to/WP/tests/phpunit/includes/bootstrap.php:95) in /path/to/WP/src/wp-includes/load.php on line 136

Warning: Cannot modify header information - headers already sent by (output started at /path/to/WP/tests/phpunit/includes/bootstrap.php:95) in /path/to/WP/src/wp-includes/load.php on line 137

Your server is running PHP version 5.4.20 but WordPress 5.3-alpha-45282-src requires at least 5.6.20.

Exit code is 0

I would like to propose to exit with an error code of 1 instead which would make sure that automated builds with incompatible PHP/WP versions will in actual fact fail in this situation.

While I realize that WP 5.2 is very close to release, I also believe this fix should still be included in WP 5.2.

The (very simple) patch which I'm attaching to this ticket should take care of this and would result in the following output:

Your server is running PHP version 5.4.20 but WordPress 5.3-alpha-45282-src requires at least 5.6.20.

Exit code is 1

Pinging @jorbin @desrosj (Hoping I'm pinging the right people. Please forgive me if I got it wrong)

[jrf]

Proposed patch

[spacedmonkey]

We should use wp_die here instead.

[jrf]

Replying to spacedmonkey:

We should use wp_die here instead.

I'm not so sure as wp_die() will display HTML and presume WP can be loaded (at least the minimum), while when requirements are not met, that may well not be the case.

[jrf]

For now, I would propose patch 47169-alternative-for-wp-5.2.patch​ to be committed to the WP 5.2 branch and committing patch 47169.patch​ to trunk early. If needs be, that can then be fine-tuned during the 5.3 dev-cycle.

[jeremyfelt]

I think something like 47169.patch​ will make sense for a 5.2.1 or 5.3 release. I'd suggest moving this ticket to a future milestone and then iterating on the patch.

It doesn't seem immediately critical that tests for PHP 5.4 work for WP 5.2 today. What impact does this have outside of our own build tooling/testing for the near term?

[ocean90]

I agree that this can wait, moving to 5.2.1 for now.

[johnjamesjacoby]

Related: #46594.

47169-alternative-for-wp-5.2.patch​ seems fine enough for now'ish. I agree with @spacedmonkey that ultimately using wp_die() would be more consistent with other similar fatals. See: wp_maintenance(), wp-load.php, etc...

If there's a reason not to use wp_die(), can you detail what that is here @jrf?

What impact does this have outside of our own build tooling/testing for the near term?

Technically, nothing yet... but only because none of the code loaded by the time this part gets executed contains anything that is version specific (beyond the lowest required) that would trigger a fatal error. When the day comes there is version-incompatible code, this will need to be addressed somehow.

[jrf]

Replying to jeremyfelt:

What impact does this have outside of our own build tooling/testing for the near term?

Well, there are plenty of plugins and themes in the wider WP sphere which base their unit tests on the WP unit test suite.
If they have a PHP < 5.6.30 build in combination with WP trunk/latest/master, their builds will currently pass, while they have no clue that their unit tests aren't actually being executed.

Of course, one expects devs in the WP sphere to keep up with current developments, but not everyone does and not all the time.

[spacedmonkey]

I think that the patch 47169.patch seems reasonable to me, but I haven't tested it yet.

I think it is reasonable to get this in 5.2 (maybe 5.2.1) and we can change over to use wp_die if possible in 5.3+.

Having a quick look at function, not sure we could use wp_die for PHP errors, but maybe we could use in for database errors, as other parts of core, already do that (wp-db.php already calls wp_die).

[jeremyfelt]

Replying to jrf:

Of course, one expects devs in the WP sphere to keep up with current developments, but not everyone does and not all the time.

Very true. Would it be worth publishing a short dev-note on make/core with a warning?

I don't think there's any issue with things going into trunk today, but I'm really wary of anything landing in 5.2 on release day beyond the about page. Even yesterday, a change to tests/ would feel more comfortable than today. :)

[jrf]

Replying to jeremyfelt:

... but I'm really wary of anything landing in 5.2 on release day beyond the about page. Even yesterday, a change to tests/ would feel more comfortable than today. :)

Let me get a Travis build running to demonstrate that patch 2 as proposed for 5.2 has no relevant impact.

[jrf]

Travis build testing patch 2: ​https://travis-ci.org/jrfnl/develop.wordpress/builds/529400832

[jrf]

Alternative patch for the WP 5.2 branch which only impacts the unit tests.

[jrf]

Patch updated for a minor CS issue - new Travis build: ​https://travis-ci.org/jrfnl/develop.wordpress/builds/529407293

[jorbin]

As discussed in core, we are going to err on the side of shipping with a bug rather than making a late change that hasn't had much testing.

Let's land the proper fix of https://core.trac.wordpress.org/attachment/ticket/47169/47169.patch in trunk tomorrow with an aim for backporting quickly so it is in 5.2.1

[SergeyBiryukov]

Replying to spacedmonkey:

I think that the patch 47169.patch seems reasonable to me, but I haven't tested it yet.

I think it is reasonable to get this in 5.2 (maybe 5.2.1) and we can change over to use wp_die if possible in 5.3+.

47169.2.patch​ changes these messages to use wp_die(), for consistency with what was done for other functions:

• [44827] dead_db()
• [45019] wp_maintenance()
• [45030] wp_check_mysql_version()

However, this also means that wp-includes/functions.php needs to remain parsable by earlier PHP versions going forward if we ever decide to switch to 5.6+ or 7+ syntax.

47169.patch​ seems like a quick win, maybe that would be enough here?

[jrf]

FYI: Tested @SergeyBiryukov's patch with a command-line plugin unit test run and found working (exit code 1 is thrown with a nice error message without PHP errors).

However, this also means that wp-includes/functions.php needs to remain parsable by earlier PHP versions going forward if we ever decide to switch to 5.6+ or 7+ syntax.

That seems like a pretty big trade-off for a slightly nicer message layout.

We may need to revisit when this check is done and whether the message should be translatable, rather sooner than later.

Just as an example: With the improvements in PHP 5.5's Internationalization extension, we could finally solve the remaining unicode issues, but if messages like this need to stay translatable, we are doomed to keep using getText.

[spacedmonkey]

A safe course, might to be to only use wp_die for the MySQL check. See 47169.3.patch. This would mean would make changes to functions.php to be PHP 5.6/7 but get a nice error for mysql at least.

@jrf Can you test my patch please.

[jrf]

@spacedmonkey Just looking at the patch now. May I ask why you've set exit to false in the $args and then do an exit( 1 ) on the line after wp_die() ?
Also wondering if it should be made explicit in the $args that response should be 500 ?

[spacedmonkey]

@jrf Called exit = false means die isn't called see ​this. This means, that exit( 1 ) is called instead of die which is the point here. Error code 500 doesn't need to be passed here, as wp_die defaults to it.

The wp_die function is function for different outputs and use cases, such as xml, json, jsonp, ajax and a nice formatted html page. It is also filterable and is usable by WP CLI as well, so that errors are displayed there too.

[earnjam]

I like the approach in 47169.3.patch​. It gives the advantage of better formatting in certain situations without tying us down and preventing more modern syntax in wp-includes/functions.php, which is a pretty large file.

[desrosj]

@jrf Does the approach in 47169.3.patch​ work sufficiently in your testing?

[SergeyBiryukov]

Replying to spacedmonkey:

Called exit = false means die isn't called see ​this. This means, that exit( 1 ) is called instead of die which is the point here.

The unit tests suite uses _wp_die_handler_exit()​ as a handler during installation, which already has exit( 1 ). However, I guess it wouldn't hurt to add it here explicitly as well.

[SergeyBiryukov]

In 45350:

Build/Test Tools: Use a non-zero exit code in wp_check_php_mysql_versions() when minimum PHP or MySQL requirements are not met.

This allows automated scripts and test suites to interpret the result correctly.

Props jrf, spacedmonkey.
Fixes #47169.

[SergeyBiryukov]

In 45351:

Build/Test Tools: Use a non-zero exit code in wp_check_php_mysql_versions() when minimum PHP or MySQL requirements are not met.

This allows automated scripts and test suites to interpret the result correctly.

Props jrf, spacedmonkey.
Merges [45350] to the 5.2 branch.
Fixes #47169.