Fix #4437: Allow child environment detectors to override false #4466

reubenmoes · 2022-01-28T23:46:32Z

…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

…e false is returned.

danepowell · 2022-02-28T22:22:39Z

src/Robo/Common/EnvironmentDetector.php

@@ -339,7 +339,7 @@ private static function getSubclassResults($functionName) {
        $results[] = call_user_func([$detector, $functionName]);
      }
    }
-    return array_filter($results);
+    return $results;


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.

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 · 2022-05-27T16:36:19Z

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.

fix acquia#4437 - If custom environment detector returns FALSE, ensur…

d5f9e06

…e false is returned.

danepowell changed the title ~~fix acquia#4437 - If custom environment detector returns FALSE, ensur…~~ Fix #4437: Allow child environment detectors to override false Feb 28, 2022

danepowell reviewed Feb 28, 2022

View reviewed changes

danepowell closed this May 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4437: Allow child environment detectors to override false #4466

Fix #4437: Allow child environment detectors to override false #4466

reubenmoes commented Jan 28, 2022 •

edited

Loading

danepowell Feb 28, 2022

reubenmoes Feb 28, 2022 •

edited

Loading

danepowell commented May 27, 2022

Fix #4437: Allow child environment detectors to override false #4466

Fix #4437: Allow child environment detectors to override false #4466

Conversation

reubenmoes commented Jan 28, 2022 • edited Loading

danepowell Feb 28, 2022

Choose a reason for hiding this comment

reubenmoes Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

danepowell commented May 27, 2022

reubenmoes commented Jan 28, 2022 •

edited

Loading

reubenmoes Feb 28, 2022 •

edited

Loading