Port Environment Indicator Variables sub-module to Drupal 8

Sadly I won't be able to work on this. If anyone is interested in working on it in a sandbox,
I can review the code gladly.

From my perspective, we don't we really need a 1-1 port of the Environment Indicators Variables sub-module in Drupal 8. It doesn't seem like there are plans to port the variable module to Drupal 8 due to the fact that we have configuration management in Drupal 8, so we can't rely on that module as a dependency like we did in Drupal 7. I think it makes the most sense to utilize core's configuration management system and a module like config_split to handle importing and storing different versions of the relevant configuration item. If we go down that path, then really all thats needed is to add a form that allows a user to create the required configuration item.

I attached a patch that does that via a sub-module using the old environment_indicator_variables name space. I don't actually think that name space makes sense in Drupal 8, but I left it in for now. If we want to enforce some type of dependency on config_split, then leaving this as a submodule and re-namespacing it seems like the best way forward to me. If we don't want to add a dependency, I think it makes a lot more sense to move what's in this patch into the parent environment indicator module. My preference is for the latter as we don't need to dictate how a site manages configuration between environments, even if config_split seems like the de facto choice at the moment.

Let me know your preference and I'm happy to move it along in either direction!

Thanks for taking the time to do this contribution @zweishar. I think that we can rename the sub module to something like environment_indicator_ui.

Also, I made some comments on the patch.

1. +++ b/modules/environment_indicator_variables/environment_indicator_variables.module
   @@ -0,0 +1,13 @@
   +/**
   + * Implements hook_uninstall().
   + */
   +function environment_indicator_variables_uninstall() {
   +  \Drupal::configFactory()->getEditable('environment_indicator.indicator')->delete();
   +}
   

   I don't think this is necessary.

2. +++ b/modules/environment_indicator_variables/environment_indicator_variables.routing.yml
   @@ -0,0 +1,7 @@
   +  path: '/admin/config/development/environment-indicator-variables/settings'
   

   What about /admin/config/development/environment-indicator/variables instead?

3. +++ b/modules/environment_indicator_variables/src/Form/EnvironmentIndicatorVariablesSettingsForm.php
   @@ -0,0 +1,82 @@
   +      '#default_value' => $config->get('name') ? : [],
   

   It seems you are always using [] as a default value. I think that

   '#default_value' => $config->get('name'),
   

   may me more appropriate.

4. +++ b/modules/environment_indicator_variables/src/Form/EnvironmentIndicatorVariablesSettingsForm.php
   @@ -0,0 +1,82 @@
   +    $form['fg_color'] = [
   +      '#type' => 'textfield',
   +      '#title' => $this->t('Foreground Color'),
   +      '#description' => $this->t('Enter a hex value for the foreground color of the admin toolbar. Example: #4298f4.'),
   +      '#default_value' => $config->get('fg_color') ? : [],
   +    ];
   +    $form['bg_color'] = [
   +      '#type' => 'textfield',
   +      '#title' => $this->t('Background Color'),
   +      '#description' => $this->t('Enter a hex value for the background color of the admin toolbar. Example: #4298f4.'),
   +      '#default_value' => $config->get('bg_color') ? : [],
   +    ];
   

   We should probably have a color field with a color picker. I believe the environments form contain an example of how to do this.

No problem and thanks for thorough code review e0ipso.

Here's an updated patch that addresses the issues you identified.

Here's one with updated help text that makes a bit more sense given the move towards a color field.

Merged! Thanks.