Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Hi!
Nice to see a Drupal 8 version of this module. Just a request to ask if the Environment Indicator Variables sub-module could be added to the list to port to Drupal 8. It makes it easier to keep the settings for all environment the same across environments.
Thanks for considering this!
Frederick
Comments
Comment #2
e0ipsoSadly 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.
Comment #3
e0ipsoComment #4
zweishar CreditAttribution: zweishar at Isovera commentedFrom 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!
Comment #5
zweishar CreditAttribution: zweishar at Isovera commentedComment #6
zweishar CreditAttribution: zweishar at Isovera commentedComment #7
e0ipsoThanks 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.
I don't think this is necessary.
What about
/admin/config/development/environment-indicator/variables
instead?It seems you are always using
[]
as a default value. I think thatmay me more appropriate.
We should probably have a color field with a color picker. I believe the environments form contain an example of how to do this.
Comment #8
zweishar CreditAttribution: zweishar at Isovera commentedNo problem and thanks for thorough code review e0ipso.
Here's an updated patch that addresses the issues you identified.
Comment #9
zweishar CreditAttribution: zweishar at Isovera commentedHere's one with updated help text that makes a bit more sense given the move towards a color field.
Comment #11
e0ipsoMerged! Thanks.