_drupal_log_error() returns a 0 exit code on errors [#2927012]

Problem/Motivation

When _drupal_log_error() is called with a fatal error, the PHP process exits with a zero:

  if (PHP_SAPI === 'cli') {
    if ($fatal) {
      // When called from CLI, simply output a plain text message.
      // Should not translate the string to avoid errors producing more errors.
      $response->setContent(html_entity_decode(strip_tags(SafeMarkup::format('%type: @message in %function (line %line of %file).', $error))) . "\n");
      $response->send();
      exit;
    }
  }

This causes CLI tools like Behat to exit 0 even though a fatal was raised, confusing CI tools that rely on the exit code.

I'd guess that just exit(1); would be enough here.

Proposed resolution

Remaining tasks

User interface changes

None

API changes

CLI tools that cause a fatal error now exit with an error status, if they use Drupal's default error handler.

Data model changes

None

Release notes snippet

CLI tools that cause a fatal error now exit with an error status, if they use Drupal's default error handler.

Comment	File	Size	Author
#30	2927012-30.patch	2.35 KB	deviantintegral
#30	8.7.x: PHP 7.2 & MySQL 5.5 26,333 pass
#28	2927012-28.patch	2.42 KB	alexpott
#28	8.7.x: PHP 7.2 & MySQL 5.5 25,117 pass
#28	2927012-28.test-only.patch	1.89 KB	alexpott
#28	8.7.x: PHP 7.2 & MySQL 5.5 25,116 pass, 1 fail
#28	25-28-interdiff.txt	3.15 KB	alexpott
#25	2927012.24-log-error-exit-code.patch	3.49 KB	deviantintegral
#25	8.7.x: PHP 7.2 & MySQL 5.5 25,115 pass
#22	2927012.20-22-log-error-exit-code.txt	2.38 KB	deviantintegral
#22	2927012.22-log-error-exit-code.patch	3.49 KB	deviantintegral
#22	8.6.x: PHP 7 & MySQL 5.5 23,439 pass PHP 7.2 & MySQL 5.5 Patch Failed to Apply 8.7.x: PHP 7.2 & MySQL 5.5 Patch Failed to Apply
#20	2927012.20-log-error-exit-code.patch	2.93 KB	deviantintegral
#20	8.6.x: PHP 7 & MySQL 5.5 23,422 pass
#12	2927012.12-log-error-exit-code.patch	2.85 KB	deviantintegral
#12	8.5.x: PHP 7 & MySQL 5.5 23,169 pass, 2 fail PHP 7.0.x-dev & MySQL 5.5 23,173 pass PHP 7.2 & MySQL 5.5 23,400 pass 8.6.x: PHP 7 & MySQL 5.5 23,446 pass
#10	2927012.10-log-error-exit-code.patch	2.85 KB	deviantintegral
#10	8.5.x: PHP 7 & MySQL 5.5 23,142 pass, 1 fail
#8	2927012.8-log-error-exit-code.patch	2.89 KB	deviantintegral
#8	8.5.x: PHP 7 & MySQL 5.5 Unable to generate test groups
#3	_drupal_log_error-return-2927012-2.patch	532 bytes	arunkumark
#3	8.5.x: PHP 7 & MySQL 5.5 22,866 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

29 November 2017 at 01:18

deviantintegral created an issue. See original summary.

Comment #2

arunkumark

Coimbatore

CreditAttribution: arunkumark as a volunteer and at DrupalPartners for CodeExpertz commented 29 November 2017 at 07:51

Assigned:

Unassigned

» arunkumark

Comment #3

arunkumark

Coimbatore

CreditAttribution: arunkumark as a volunteer and at DrupalPartners for CodeExpertz commented 29 November 2017 at 07:53

Status:

Active

» Needs review

File	Size
_drupal_log_error-return-2927012-2.patch	532 bytes
8.5.x: PHP 7 & MySQL 5.5 22,866 pass

I have created the patch for returning the true value.

Comment #4

arunkumark

Coimbatore

CreditAttribution: arunkumark as a volunteer and at DrupalPartners for CodeExpertz commented 29 November 2017 at 07:57

Assigned:

arunkumark

» Unassigned

Comment #5

deviantintegral CreditAttribution: deviantintegral at Lullabot commented 3 December 2017 at 21:30

Looks fine - any thoughts on how to add test coverage for this?

Comment #6

dawehner

German

CreditAttribution: dawehner at Chapter Three commented 4 December 2017 at 08:50

I fear we would need a test CLI script which calls some code, which causes the fatal.

Comment #7

m4olivei

he/him

English

Grimsby, ON

CreditAttribution: m4olivei commented 13 December 2017 at 21:33

+1 to this, I've seen two instances of Behat silently failing as a result of changes in Behat's dependencies.

@dawehner is there precedent for what you propose?

Comment #8

deviantintegral CreditAttribution: deviantintegral at Lullabot commented 16 January 2018 at 17:00

File	Size
2927012.8-log-error-exit-code.patch	2.89 KB
8.5.x: PHP 7 & MySQL 5.5 Unable to generate test groups

Here's a "unit" test that covers this using the Process component. Open to suggestions if log-exit.php should live somewhere else.

Comment #9

16 January 2018 at 17:03

Status:

Needs review

» Needs work

The last submitted patch, 8: 2927012.8-log-error-exit-code.patch, failed testing. View results

Comment #10

deviantintegral CreditAttribution: deviantintegral at Lullabot commented 16 January 2018 at 17:10

Status:

Needs work

» Needs review

File	Size
2927012.10-log-error-exit-code.patch	2.85 KB
8.5.x: PHP 7 & MySQL 5.5 23,142 pass, 1 fail

I thought that test discovery only tried to inspect files ending in Test, but apparently not. This renames the test script to log-error.sh, which is annoying for editors that try to render as bash but at least matches what core does in core/scripts.

Comment #11

16 January 2018 at 17:53

Status:

Needs review

» Needs work

The last submitted patch, 10: 2927012.10-log-error-exit-code.patch, failed testing. View results

Comment #12

deviantintegral CreditAttribution: deviantintegral at Lullabot commented 16 January 2018 at 18:04

Status:

Needs work

» Needs review

File	Size
2927012.12-log-error-exit-code.patch	2.85 KB
8.5.x: PHP 7 & MySQL 5.5 23,169 pass, 2 fail PHP 7.0.x-dev & MySQL 5.5 23,173 pass PHP 7.2 & MySQL 5.5 23,400 pass 8.6.x: PHP 7 & MySQL 5.5 23,446 pass

File

Size

2927012.12-log-error-exit-code.patch

2.85 KB

8.6.x:
PHP 7 & MySQL 5.5 23,446 pass

Fixing a namespace typo that caused run-tests to fail.

Comment #13

m4olivei

he/him

English

Grimsby, ON

CreditAttribution: m4olivei commented 16 January 2018 at 19:24

Status:

Needs review

» Reviewed & tested by the community

Looks great! Test makes sense, minimal POC script that ultimately calls _drupal_log_error(), then using Symfony Process component to assert the exit status.

Comment #14

16 January 2018 at 19:24

Version:

8.5.x-dev

» 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Comment #15

deviantintegral CreditAttribution: deviantintegral at Lullabot commented 17 January 2018 at 14:32

This bug fix would be great to get in 8.5.x and even 8.4.x if another point release is going to be made.

Comment #16

24 January 2018 at 04:56

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, 12: 2927012.12-log-error-exit-code.patch, failed testing. View results

Comment #17

deviantintegral CreditAttribution: deviantintegral at Lullabot commented 24 January 2018 at 22:52

Status:

Needs work

» Reviewed & tested by the community

Comment #18

deviantintegral CreditAttribution: deviantintegral at Lullabot commented 24 January 2018 at 22:52

Looks like the prior test failure was unrelated. Tests reran and passed.

Comment #19

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce commented 30 January 2018 at 23:15

Status:

Reviewed & tested by the community

» Needs work

+++ b/core/modules/system/tests/src/Unit/Errors/DrupalLogErrorTest.php
@@ -0,0 +1,25 @@
+namespace Drupal\Tests\system\Unit\Errors;

This test should be in the core namespace not system since the function under test is core's not system's. Ie. Drupal\Tests\Core\Error

```
+++ b/core/modules/system/tests/src/Unit/Errors/log-exit.sh
```
I think we should keep this .php - we have more and more scripts going in as .php which makes everything easy. Also let's put this in core/tests/Drupal/Tests/Core/Error/fixtures
I also think we exit from this script if we're not in running from CLI. Ie.
```
if (PHP_SAPI !== 'cli') {
  return;
}
```

Comment #20

deviantintegral CreditAttribution: deviantintegral at Lullabot commented 31 January 2018 at 20:01

Status:

Needs work

» Needs review

File	Size
2927012.20-log-error-exit-code.patch	2.93 KB
8.6.x: PHP 7 & MySQL 5.5 23,422 pass

This moves the test and adds the SAPI check.

I can't find any examples of fixtures using the .php suffix - the closest is something like core/tests/Drupal/Tests/Core/Form/fixtures/form_base_test.inc. If the fixture is named log-exit.php this ReflectionException is thrown:

$ core/scripts/run-tests.sh Error
ReflectionException: Class Drupal\Tests\Core\Error\fixtures\log-exit does not exist in /var/www/docroot/core/modules/simpletest/src/TestDiscovery.php:329
Stack trace:
#0 /var/www/docroot/core/modules/simpletest/src/TestDiscovery.php(329): ReflectionClass->__construct('Drupal\\Tests\\Co...')
#1 /var/www/docroot/core/modules/simpletest/src/TestDiscovery.php(177): Drupal\simpletest\TestDiscovery::getTestInfo('Drupal\\Tests\\Co...', '')
#2 /var/www/docroot/core/modules/simpletest/simpletest.module(590): Drupal\simpletest\TestDiscovery->getTestClasses(NULL, Array)
#3 /var/www/docroot/core/scripts/run-tests.sh(1103): simpletest_test_get_all(NULL, Array)
#4 /var/www/docroot/core/scripts/run-tests.sh(145): simpletest_script_get_test_list()
#5 {main}%

Comment #21

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce commented 31 January 2018 at 20:16

Ah that's our magical test discovery assuming everything is a test :)

We have core/tests/fixtures for PHP fixtures that should not be discovered. Imo we should file an issue to improve TestDiscovery to ignore things in fixture directories. But for now we can move this file there and use php.

+++ b/core/tests/Drupal/Tests/Core/Error/fixtures/log-exit.sh
@@ -0,0 +1,48 @@
+ * @see \Drupal\Tests\system\Kernel\Error\DrupalLogErrorTest

Needs updating to the new namespace.

Comment #22

deviantintegral CreditAttribution: deviantintegral at Lullabot commented 5 February 2018 at 23:40

File	Size
2927012.22-log-error-exit-code.patch	3.49 KB
8.6.x: PHP 7 & MySQL 5.5 23,439 pass PHP 7.2 & MySQL 5.5 Patch Failed to Apply 8.7.x: PHP 7.2 & MySQL 5.5 Patch Failed to Apply
2927012.20-22-log-error-exit-code.txt	2.38 KB

File

Size

2927012.22-log-error-exit-code.patch

3.49 KB

8.6.x:
PHP 7 & MySQL 5.5 23,439 pass
PHP 7.2 & MySQL 5.5 Patch Failed to Apply

8.7.x:
PHP 7.2 & MySQL 5.5 Patch Failed to Apply

2927012.20-22-log-error-exit-code.txt

2.38 KB

Good catch on the namespace comment.

This patch fixes that along with moving the test script. As well, it explicitly tests the PHP output so errors like invalid PHP or a bad path don't give us a false pass.

Comment #23

deviantintegral CreditAttribution: deviantintegral at Lullabot commented 6 February 2018 at 15:10

Comment #24

6 February 2018 at 15:10

Version:

8.6.x-dev

» 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Comment #25

deviantintegral CreditAttribution: deviantintegral at Lullabot commented 24 October 2018 at 17:07

File	Size
2927012.24-log-error-exit-code.patch	3.49 KB
8.7.x: PHP 7.2 & MySQL 5.5 25,115 pass

Straight reroll against 8.7.x.

Comment #26

voleger

he/his

Ukrainian

Ukraine, Rivne

CreditAttribution: voleger as a volunteer and at GOLEMS GABB, Drupal Ukraine Community for Drupal Ukraine Community, GOLEMS GABB commented 26 October 2018 at 11:16

Comment #27

mtift

he/him

English

Minnesota, USA

CreditAttribution: mtift at Lullabot commented 28 February 2019 at 17:51

Status:

Needs review

» Reviewed & tested by the community

Looks good to me

Comment #28

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce commented 28 February 2019 at 23:49

Status:

Reviewed & tested by the community

» Needs review

File	Size
25-28-interdiff.txt	3.15 KB
2927012-28.test-only.patch	1.89 KB
8.7.x: PHP 7.2 & MySQL 5.5 25,116 pass, 1 fail
2927012-28.patch	2.42 KB
8.7.x: PHP 7.2 & MySQL 5.5 25,117 pass

Well can make the test a bit more contained, the script a bit less complex and also less PHP code lying around.

Comment #29

1 March 2019 at 00:53

The last submitted patch, 28: 2927012-28.test-only.patch, failed testing. View results

Comment #30

deviantintegral CreditAttribution: deviantintegral at Lullabot commented 1 March 2019 at 17:32

File	Size
2927012-30.patch	2.35 KB
8.7.x: PHP 7.2 & MySQL 5.5 26,333 pass

TIL'ed about PhpProcess. Nice!

#28 looks fine, but $path is now unused. It's such a minor fix that if tests pass I will bump this to RTBC.

Comment #31

deviantintegral CreditAttribution: deviantintegral at Lullabot commented 1 March 2019 at 19:13

Status:

Needs review

» Reviewed & tested by the community

Comment #32

1 March 2019 at 19:13

Version:

8.7.x-dev

» 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Comment #33

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 31 May 2019 at 05:08

Can we get a change record for this - even though its a bug, people might have tests that now start (correctly!) failing and it would be good if they went to list-changes if they saw this.

Comment #34

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce commented 31 May 2019 at 08:45

Created https://www.drupal.org/node/3058400

Comment #35

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce commented 31 May 2019 at 08:49

Issue summary:	View changes
Issue tags:		+8.7.3 release notes

Updated the issue summary. I think we should consider this a bug fix that should go in 8.7.x so I've created the CR with those values and tagged the issue accordingly. However, if others feel we can only do this in 8.8.x, so be it, as getting this fixed is more important than arguing which release it belongs in.

Comment #36

deviantintegral CreditAttribution: deviantintegral at Lullabot commented 31 May 2019 at 12:33

Agreed this should go in 8.7.x, especially as this affects fatal errors.

Change record looks good to me.

Comment #37

31 May 2019 at 20:20

larowlan committed 08879cc on 8.8.x

Issue #2927012 by deviantintegral, alexpott, arunkumark:...

Comment #38

31 May 2019 at 20:20

larowlan committed 09bfe7f on 8.7.x

Issue #2927012 by deviantintegral, alexpott, arunkumark:...

Comment #39

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 31 May 2019 at 20:21

Version:	8.8.x-dev	» 8.7.x-dev
Status:	Reviewed & tested by the community	» Fixed

Committed 08879cc and pushed to 8.8.x. Thanks!

c/p as 09bfe7fa12 to 8.7

Published change record.

Comment #40

14 June 2019 at 20:24

Status:

Fixed

» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

_drupal_log_error() returns a 0 exit code on errors

Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comments

Change records for this issue

Child issues

Related issues