WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 6 months ago

Last modified 6 months ago

#47169 closed defect (bug) (fixed)

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

Reported by: jrf Owned by: SergeyBiryukov
Milestone: 5.2.1 Priority: normal
Severity: normal Version: 3.0
Component: Build/Test Tools Keywords: has-patch 2nd-opinion
Focuses: Cc:
PR Number:

Description

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)

Attachments (4)

47169.patch (1.7 KB) - added by jrf 6 months ago.
Proposed patch
47169-alternative-for-wp-5.2.patch (1.2 KB) - added by jrf 6 months ago.
Alternative patch for the WP 5.2 branch which only impacts the unit tests.
47169.2.patch (1.9 KB) - added by SergeyBiryukov 6 months ago.
47169.3.patch (1.6 KB) - added by spacedmonkey 6 months ago.

Download all attachments as: .zip

Change History (29)

@jrf
6 months ago

Proposed patch

#1 follow-up: @spacedmonkey
6 months ago

We should use wp_die here instead.

#2 in reply to: ↑ 1 @jrf
6 months ago

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.

#3 @jrf
6 months ago

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.

#4 follow-up: @jeremyfelt
6 months ago

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?

#5 @ocean90
6 months ago

  • Milestone changed from 5.2 to 5.2.1

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

#6 @johnjamesjacoby
6 months ago

  • Milestone changed from 5.2.1 to 5.2

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.

#7 in reply to: ↑ 4 ; follow-up: @jrf
6 months ago

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.

Last edited 6 months ago by jrf (previous) (diff)

#8 follow-up: @spacedmonkey
6 months ago

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

#9 in reply to: ↑ 7 ; follow-up: @jeremyfelt
6 months ago

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

#10 in reply to: ↑ 9 @jrf
6 months ago

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
6 months ago

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

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


6 months ago

#13 @jrf
6 months ago

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

#14 @jorbin
6 months ago

  • Milestone changed from 5.2 to 5.2.1

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

#15 in reply to: ↑ 8 @SergeyBiryukov
6 months ago

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:

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?

#16 @jrf
6 months ago

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.

#17 @spacedmonkey
6 months ago

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.

#18 @jrf
6 months ago

@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 ?

#19 follow-up: @spacedmonkey
6 months ago

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

#20 @earnjam
6 months ago

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.

#21 @desrosj
6 months ago

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

#22 @SergeyBiryukov
6 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#23 in reply to: ↑ 19 @SergeyBiryukov
6 months ago

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.

#24 @SergeyBiryukov
6 months ago

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

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.

#25 @SergeyBiryukov
6 months ago

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.

Note: See TracTickets for help on using tickets.