"git describe --all" is unreliable

Hello! Thanks for taking the time to contribute to the issue queue. However in this case it seems that we already tried git rev-parse.

This method was the initial attempt and was discarded as part of #2287977: Git checkouts of tags cause PHP notice.

Any particular issues you are finding?

Thanks for the improvements! A couple of comments:

1. +++ b/environment_indicator.module
   @@ -610,17 +610,20 @@ function environment_indicator_environment_indicator_matched_indicator_alter(&$e
   +        }
   +
   +        // Checkout is a tag.
   +        else {
   

   This does not comply with the Drupal coding standards. Please check.

2. +++ b/environment_indicator.module
   @@ -610,17 +610,20 @@ function environment_indicator_environment_indicator_matched_indicator_alter(&$e
   +          $tag_branch_parts = explode('/', environment_indicator_execute_os_command('git describe --all'));
   

   We should keep the old logic that checked if the return value was empty.

I added the below to https://www.drupal.org/node/2640016 - sharing this here.

Running proc_open on a production server via the web user can be considered a security risk (and proc_open is often added to disable_functions), and can also slow down page loads or lock up the server if the command hangs for some reason (the command is blocking and there is no timeout), which happened recently to a client of ours on a locked down server (unfortunately we didn't have access to debug further).

While this functionally can be disabled by disabling the module or setting environment_indicator_git_support to FALSE, I wonder if there is a safer way to do this (at least by default).

It seems like it might (at least for the case of branch detection) to simply walk up the realpath'ed directory tree and read the contents of .git/HEAD if it exists. Not sure if people are using this functionality to identify tags - if so, this is a less attractive option (unless we want to start matching the hash against refs/* or pull in some library to help).

simply walk up the realpath'ed directory tree and read the contents of .git/HEAD if it exists

The problem is that there is no guarantee that the git root is in a particular place and that you cannot assume that Apache (or other) has read access to that. I believe that by giving the option to turn it off, it's good enough for most users when it comes to the valid proc_open concerns.

For those that it's not, they can turn it off and then implement the alter hook with the directory scanning technique.

If Apache (more accurately the php user, which is not always the Apache user) does not have access to the git root then the git executable (which will also run as the php user) would not have access to this either.

I was actually coming here to submit a similar issue.

That being said I was going to go with a simpler solution. I was hoping we could just make the command configurable and let the site choose their option.

In my case I don't like any of the listed options above and like "git describe --long --all" better and it work better for my use case.

So thoughts? Making the command configurable or giving site owners a list of options instead of trying to account for everyones needs in a single solution.

I like the idea of giving site owners a list of options.

On second thought, maybe we should remove the feature all together and recommend setting the environment_indicator_remote_release variable as documented in the samples/environment-indicator.sh

It's done. The Drupal 8 version will not support the GIT feature. Executing OS commands is a red flag that tells us that this information should be gathered some other way. Git hooks is probably the way to go.

Drupal 7 will stay as is with its shortcomings and virtues, unless there is a security issue.