PHP 7.4 notice in run-test.sh

Shouldn't this be `!empty($args['color'])` call instead of isset()?

`$args['color'] = 0` will evaluate to false in current code, but true with `isset()`.

Test failures are not related to this patch either way :)

Agree with #4, this should be !empty()

combine isset with empty, avoid all notices.

combine isset with empty, avoid all notices.

No, they are mutually exclusive, choose one or the other.

https://www.php.net/manual/en/function.empty.php
empty() does not generate a warning if the variable does not exist.

Thanks, yes I agree, empty , there's no need for a micro optimisation here as this script is not actually called thousands of times in a thread like some others are (for loops in code that is run on every bootstrap for instance).
#2770065: array_key_exists() micro-optimization
isset() being the fastest of the two, double condition is faster but out of scope for this patch.

new patch

Yup, we either need isset($args['color']) or !empty($args['color']). The last patch has just empty($args['color']) which is wrong.

I think isset($args['color']) is probably more consistent with what we've been doing elsewhere, and as @joseph.olstad mentioned I don't think we care too much about the relative performance in this case.

How about other references to elements of the $args global?

$ grep -n "if (\$args\['" scripts/run-tests.sh 
18:if ($args['help'] || $count == 0) {
23:if ($args['execute-test']) {
40:if ($args['clean']) {
61:if ($args['list']) {
99:if ($args['xml']) {
418:  if ($args['color']) {
437:  if ($args['all']) {
441:    if ($args['class']) {
460:    elseif ($args['file']) {
475:    elseif ($args['directory']) {
486:      if ($args['directory'][0] === '/') {
560:  if ($args['all']) {
668:  if ($args['verbose']) {
732:  if ($args['color']) {

...do these not have the same issue?

ok , ya oops about my last patch , how about this:

Thanks!

This is going to be tricky to review as I suppose we're stuck with only manual testing!

It looks like there's real mix of isset() and empty() / !empty() in the existing code.

Although it might be nice to be more consistent, I think in this case I'd prefer to keep the changes to a minimum, so my vote would be only change references to array elements that'll cause PHP 7.4 exceptions and leave anything else that's already there alone.

The real question here to me is why is only color affected, when we have code to ensure defaults exist in the $args array - this looks like it should solve this for all cases:

function simpletest_script_parse_args() {
  // Set default values.
  $args = array(
    'script' => '',
    'help' => FALSE,
    'list' => FALSE,
    'clean' => FALSE,
    'url' => '',
    'php' => '',
    'concurrency' => 1,
    'all' => FALSE,
    'class' => FALSE,
    'file' => FALSE,
    'directory' => '',
    'color' => FALSE,
    'verbose' => FALSE,
    'test_names' => array(),
    // Used internally.
    'test-id' => 0,
    'execute-test' => '',
    'xml' => '',
  );

I really hope I'm missing something here...

isset() and empty() are not interchangeable. isset() doesn't care about the "truthiness" of the value. It will simply return true if the object property, array key, or variable exists and is not null. empty() will do it, and more, by loosely evaluating its truthiness.

I do apologize if my comment in #4 sounded misleading. It was not against the use of isset vs !empty, but rather the semantically correct use of it. In function simpletest_script_parse_args(), you will see that the $args array is initialized and the key color is set to FALSE. So with isset($args['color]), even FALSE evaluates to true.

I cross-posted above #14. I totally agree with @longwave I think we need to investigate why color key was missing but nothing else.
Can anyone else actually reproduce this bug?

I had totally missed the initialisation of the defaults for $args - I hadn't really read the code properly at all, so I'm glad other people are :)

As for reproducing the issue, yes I think I can:

# php scripts/run-tests.sh --borked
PHP Notice:  Trying to access array offset on value of type null in /path/to/drupal-7.x/scripts/run-tests.sh on line 732
  ERROR: Unknown argument '--borked'.

# php --version
PHP 7.4.3 (cli) (built: Feb 23 2020 07:24:02) ( NTS )

Is the problem that this:

      else {                                                                       
        // Argument not found in list.                                             
        simpletest_script_print_error("Unknown argument '$arg'.");                 
        exit(SIMPLETEST_SCRIPT_EXIT_FAILURE);                                      
      } 

...is called from within simpletest_script_parse_args() before that function returns, which means that $args (with the defaults set) has not yet gone into the global scope.

So at this point:

function simpletest_script_print($message, $color_code) {                       
  global $args;                                                                 
  if ($args['color']) {                                                         
    echo "\033[" . $color_code . "m" . $message . "\033[0m";                    
  }

...isn't the global $args we're looking for, so to speak.

Simplest fix would probably be to check if it's not empty then (that should work both in the case there's nothing there and it's been initialised to the default of FALSE).

I investigated the same way and found the same issue as #19, $args is not in global scope by the time simpletest_script_print() is called in the case of invalid parameters, so the only thing we need to fix is the check inside that function. In theory we could do a more comprehensive fix and correct the scoping but practically I think we should go for the smallest fix that works.

I also agree that !empty() is correct here as FALSE should not pass the condition.

Thanks!

yes I like the minimalist approach, I was just throwing the kitchen sink in there for discussion.
+ patch 20 thanks @longwave

RTBM - thanks all!

Thanks everyone!