Environment Indicator - Githubissues

kalamuna / drupal-project

Kalamuna composer template for Drupal projects with CircleCI and Pantheon integration. (NOTE: THIS SHOULD NOT BE USED FOR NEW DRUPAL 9 SITES)

GNU General Public License v2.0

6 stars 1 forks source link

Environment Indicator #86

Open enotick opened 4 years ago

enotick commented 4 years ago

@RobLoach can you please take a look at environment indicator settings. Let's test it like this if this is ok and then we can refactor if you'd like. Thanks.

mikemccaffrey commented 4 years ago

I'm fine with installing the indicator, but I would like to clean this code up a bit:

Let remove everything that is commented out.
We have virtually no sites on acquia, so we probably don't need to check those variables.

However, there is also a bit of a fundamental issue: The colors have been chosen to discourage developers from making changes on live instead of dev, but for site editors we actually want the opposite. They should make content changes on live and instead be warned if they are making changes to dev or test.

Is there a way we can tie the indicator colors to roles or permissions so we don't alarm the site editors with a bright red admin bar on the live site?

enotick commented 4 years ago

@mikemccaffrey no the indicator is for developers, for editors we can hide it completely. I am sure you could do something like an event subscriber or preprocess and get the role there but this is generally speaking not the original purpose of the module - it was developed to work with configuration and alert people to not do config save on live. In terms of removing Acquia I think we should keep it - we may have sites on Acquia later on but more precisely this snippet has benefited from being low maintenance in terms of environment detection over years. It's taken from BLT that works with Acquia and Pantheon and the splits specifically as well as detection changes quite a bit - being able to copy paste saved us a lot of time in the past and allowed to rely on BLT community and maintainers and not re-invent the wheel. While we can clean it up - it's just easier to maintain as is - speaking from experience.

mikemccaffrey commented 4 years ago

It would just be weird to have this one single piece of code reference acquia in the entire project. If were were to support acquia with this project, things would be much more complicated.

enotick commented 4 years ago

Hmmmm I can see that... idk I think having two hostings support could be nice and I also agree that this increases CI complexity for sure. Maybe something generator could solve for us. Ok let's in the interim remove Acquia out of the picture but maybe be have it for the future stage as a goal.