Fix #4437: Allow child environment detectors to override false

[reubenmoes]

…e false is returned.

Motivation

Fixes #4437
There is currently no way to allow a custom environment indicator to say that an environment is not active. This was due to the parent filtering out any results if they are 'false'. This results in config split applying the 'local' config splits in any remote environment that is not on Acquia.

Proposed changes

reserve the results from the custom environment indicator whether they return true or false. Then evaluate what the subclasses have returned. The assumption is that if we have a custom environment detector, that should override the capability and returning true/false within the custom environment indicator should be the end result, and it should not need to continue to the parent to check.

Alternatives considered

I'm a bit unsure about the logic used. open to suggestions.

Testing steps

Create a custom environment indicator and put it in a state where isLocalEnv returns false. Test in a non-acquia environment. Easiest to test with config split. User should see that local is not active (or local config split is not active)

Example custom environment detector to test with.

<?php

namespace ConcertSetup\Blt\Plugin\EnvironmentDetector;

use Acquia\Blt\Robo\Common\EnvironmentDetector;

class CustomEnvironmentDetector extends EnvironmentDetector {

  public static function isLocalEnv() {  
  
     // This will return false, because we have set isProduction() to return true.
    return !self::isStageEnv() && !self::isProdEnv() && !self::isDevEnv();
  }

  /**
   * Is stage (on google cloud custom hosting).
   */
  public static function isStageEnv() {
    return getenv('ENVIRONMENT_ID') == 'stage';
  }

  /**
   * Is stage (on google cloud custom hosting).
   */
  public static function isDevEnv() {
    return getenv('ENVIRONMENT_ID') == 'dev';
  }

  /**
   * Is Prod (on google cloud custom hosting).
   */
  public static function isProdEnv() {
    return true;
    return $_ENV['ENVIRONMENT_ID'] == 'prod';
  }

}

Merge requirements

• Major change, Minor change, Bug, Enhancement, and/or Chore label applied
• Manual testing by a reviewer

[danepowell]

The doc block for this function needs to be updated to account for false results being returned.

This significantly changes the behavior of multiple detectors being called, doesn't it? Previously, if any detector returned true that detector would 'win', i.e. the parent method would return true. Now, the result is going to vary by method. For instance, what should happen if one detector returns false and another returns true for isStageEnv or isProdEnv? I think this behavior needs to be consistent, and it needs to be documented.

[reubenmoes]

oh i have some inconsistency here. I had a change of approach last minute. Which one would be preferred?

---

option 1:

should happen if one detector returns false and another returns true for isStageEnv or isProdEnv
This would result in it being overall false.
If there is only one environment detector returning true - would be true
If there is only one environment detector returning false - would be false (and would not continue to call the parent method)

    if ( !empty($results) ) {
      return !in_array(FALSE, $results);
    }

---

Option 2: (seems like this would be more consistent with what was previously in place)

should happen if one detector returns false and another returns true for isStageEnv or isProdEnv
This would result in it being overall true.
If there is only one environment detector returning true - would be true
If there is only one environment detector returning false - would be false (and would not continue to call the parent method)

    if ( !empty($results) ) {
      return in_array(TRUE, $results);
    }

[danepowell]

I don't see any clean solution to this that preserves backwards compatibility, sorry. If we can do this in a way that preserves BC I'd be willing to reconsider.