Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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:
- 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.
- 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.
Comment | File | Size | Author |
---|---|---|---|
#3 | drush-api-2181927-3.patch | 2.62 KB | benjifisher |
#2 | drush-api-2181927-2.patch | 2.49 KB | benjifisher |
Comments
Comment #1
benjifisher@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 likewith
(and I removed the earlier line that replaced
$vim
withescapeshellcmd($vim)
). Does that seem about right?Now I am getting errors from
exec('vim --version')
as well asdrush_shell_exec('vim --version')
. I will come back to this after restarting my computer ...Comment #2
benjifisherI am still getting errors when I try to run "vim" through
exec()
ordrush_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
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 invokedrush_escapeshellarg()
while building the string.Comment #3
benjifisherIt 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.Comment #4
rodrigoaguileraI 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.
Comment #6
benjifisher@rodrigoaguilera:
Thanks for the thorough review.