The vimrc.drush.inc commandfile is using some low level php functions in places where Drush provides useful helper functions, especially in the vimrc_tag_gen* functions. This could probably use a detailed review, but the big ones that stand out are:

  1. Using exec() instead of drush_shell_exec() (and related functions, see exec.ini) with argument placeholders - this helps ensure arguments are quoted correctly, and provides support for --debug command logging and --simulate.
  2. Using print instead of drush_log() - the latter provides better status messages, as well as the ability for automated parsing of logs. There is also drush_print(), although this is generally best reserved for concrete "output", for example something you might want to redirect to a file.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

benjifisher’s picture

@Owen Barton:

Thanks for the advice. This is my first attempt at writing a drush command, and I need all the help I can get.

As a first pass at replacing exec(), I replaced code like

  exec("$vim --version", $output, $retval);
  if ($retval) {...}

with

  $success = drush_shell_exec('%s --version', $vim);
  $output = drush_shell_exec_output();
  if (!$success) {...}

(and I removed the earlier line that replaced $vim with escapeshellcmd($vim)). Does that seem about right?

Now I am getting errors from exec('vim --version') as well as drush_shell_exec('vim --version'). I will come back to this after restarting my computer ...

benjifisher’s picture

Issue summary: View changes
Status: Active » Needs work
FileSize
2.49 KB

I am still getting errors when I try to run "vim" through exec() or drush_shell_exec(). Rebooting my computer did not help, but specifying --vim-command=mvim or --vim-command=/usr/bin/vim does work. It looks like some odd configuration on my account, not a problem with the script.

I am attaching a patch that addresses #1. Since I have not done anything yet about #2, I am marking the issue NW instead of NR. Note the snippet

  // Do not pass $options as an argument to drush_shell_exec() because then it
  // will get wrapped in single quotes by drush_escapeshellarg().
  drush_shell_exec("%s -f %s $options %s", $ctags, $tag_file, $path);

In one test, $options is -R --totals --extra=+f --langmap=php:+.module.install.inc.profile.theme.engine,javascript: --php-kinds=-v --exclude=.git --exclude=tests --exclude=Tests --exclude=Test --exclude=Test.php. This is all hard-coded (no user-supplied strings) but I suppose I could invoke drush_escapeshellarg() while building the string.

benjifisher’s picture

Status: Needs work » Needs review
FileSize
2.62 KB

It seems that I already mostly used drush_log() already. I searched for print\|echo ... if you are reading this, you should already know a little about regular expressions ... and found just one. Quick change, quick test, and now I think this is ready for a second set of eyes.

rodrigoaguilera’s picture

Status: Needs review » Reviewed & tested by the community

I had a look at the code and looks OK.
I tested the affected drush commands with and without the patch on drush 7 and drush 6 and I experienced the same aoutcome so looks good.

  • benjifisher committed c9d30f9 on 7.x-1.x
    Issue #2181927 by Owen Barton, benjifisher, rodrigoaguilera: Improve...
benjifisher’s picture

Status: Reviewed & tested by the community » Fixed

@rodrigoaguilera:

Thanks for the thorough review.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.