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.
"git describe --all" is an unreliable method if you never commit from your release and prod environments. this appears to be a much better method. I haven't it found it wrong yet.
Patch to follow.
Comments
Comment #2
WorldFallz CreditAttribution: WorldFallz commentedand, here it is.
Comment #3
e0ipsoHello! 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?
Comment #4
WorldFallz CreditAttribution: WorldFallz commentedmy bad for not finding that issue-- I did search though, lol. And yep, I'm having an issue with my release/qa environment.
"git describe --all" returns "heads/master" even though I'm working from a checkout of the 'release' branch. And "git rev-parse --abbrev-ref HEAD" correctly returns 'release'.
this might require a bit more logic around whether a branch or a tag is checked out.
Now that I have that previous issue to reference, I'll take a deeper drive and see why my release branch is returning the wrong info and if I can come up with a more fool proof method.
and thanks for such a quick reply!
Comment #5
WorldFallz CreditAttribution: WorldFallz commentedhow about something like this.
Comment #6
WorldFallz CreditAttribution: WorldFallz commentedNow with less stray white space, lol.
Comment #7
WorldFallz CreditAttribution: WorldFallz commentedSorry for the churn-- doing a lot of testing with different branch and tag environments. here's the latest.
Comment #8
e0ipsoThanks for the improvements! A couple of comments:
This does not comply with the Drupal coding standards. Please check.
We should keep the old logic that checked if the return value was empty.
Comment #9
WorldFallz CreditAttribution: WorldFallz commentedthanks for the quick review! I should have an updated patch later today.
Comment #10
WorldFallz CreditAttribution: WorldFallz commentedhere it is. I've added back the empty check.
Also, I ran the module through coder, and while there's lots of results, none reference the patched lines. what is it about comment #8 bullet 1 that you'd like me to change?
Comment #11
WorldFallz CreditAttribution: WorldFallz commentedwrong patch, sheesh. Here's the right one.
Comment #12
Owen Barton CreditAttribution: Owen Barton at CivicActions commentedI 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).
Comment #13
e0ipsoThe 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.
Comment #14
Owen Barton CreditAttribution: Owen Barton at CivicActions commentedIf 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.
Comment #15
ericduran CreditAttribution: ericduran at NBCUniversal commentedI 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.
Comment #16
e0ipsoI like the idea of giving site owners a list of options.
Comment #17
e0ipsoOn second thought, maybe we should remove the feature all together and recommend setting the
environment_indicator_remote_release
variable as documented in thesamples/environment-indicator.sh
Comment #18
e0ipsoIt'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.