Problem/Motivation

We execute run-test.sh during CI runs. After an upgrade to PHP 8.1 the test pipeline did not fail although no tests were executed and there was a fatal TypeError.

Steps to reproduce

1. Remove the "static" keyword from a Simpletest test case method getInfo()
2. Execute run-tests.sh to get an error like "TypeError: call_user_func(): Argument #1 ($callback) must be a valid callback, non-static method PrivatemsgEMailNotifyTestCase::getInfo() cannot be called statically in simpletest_test_get_all() (line 372 of /var/www/html/modules/simpletest/simpletest.module)." For example: ddev exec php ./scripts/run-tests.sh --verbose Jobiqo
3. notice that the exit code is 0 for success, which is bad because that indicates that there was no error.

Proposed resolution

Exit with status code 1 in _drupal_log_error() for CLI scripts.

Remaining tasks

Merge request review

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3420076

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

klausi created an issue. See original summary.

klausi’s picture

Issue summary: View changes
Status: Active » Needs review

Merge request created.

poker10’s picture

Thanks for reporting/working on this. The same change seems to be already in D10, see: #2927012: _drupal_log_error() returns a 0 exit code on errors, so I think this looks good.

Not sure if we can easily backport the test from D10, as we do not have PhpProcess. Any thoughts? Did not have time to look at that more thoroughly yet.

klausi’s picture

Hey nice find!

We could copy the code from Symfony Process for the test, but not sure if it is worth it to try to port that somehow?

I would say we can commit this without test, since it does not affect end users and is just for command line invocations.

tibezh’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, I think we can merge this small change.

mcdruid’s picture

Issue tags: +Needs change record

I don't think we need to jump through hoops to backport the test to D7.

However, per #2927012-33: _drupal_log_error() returns a 0 exit code on errors it deserves a Change Record as it may cause a change in the behaviour of tests.

poker10’s picture

Issue tags: -Needs change record +Pending Drupal 7 commit

I have created a draft CR for D7: https://www.drupal.org/node/3438765 (it is mostly the copy from the D8 CR).

  • mcdruid committed 2b87efb3 on 7.x
    Issue #3420076 by klausi, poker10: _drupal_log_error() should return a...
mcdruid’s picture

Title: TypeError during run-tests.sh results in success exit code » _drupal_log_error() should return a non-zero exit code on errors in the cli
Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

Thanks everyone!

Status: Fixed » Closed (fixed)

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