"Awjrichards" changed the status of Wikimedia.r1176 to "fixme" and commented it.
URL: http://www.mediawiki.org/wiki/Special:Code/Wikimedia/1176#c30251

Old Status: new
> New Status: fixme

Commit summary for Wikimedia.r1176:

Initial commit of the globalcollect audit module.
This doesn't currently work, and will die as soon as it puts a stomp array 
together. Awesome.

Awjrichards's comment:

<pre>
                $cmd = "grep '<ORDERID>$order_id</ORDERID>' 
/home/khorn/logs/globalcollect/20111221_iop.log";
                
                echo $cmd . "\n";
                $ret = array();
                exec( $cmd, $ret, $errorlevel );
                $proof = array();
</pre>
You should wrap $cmd in something like escapeshellcmd() 
(http://us.php.net/manual/en/function.escapeshellcmd.php). I realize the way 
it's currently being used is benign, it is good practice and will potentially 
be crucial when you make the file name something dynamic.

<pre>
        $stomp_field_map = array( ...
</pre>
You might punch me in the face for this, but I think you should abstract out 
all or at least nearly all stomp-related stuff from both this module as well as 
DonationInterface and put it into its own library - similar to what Jeremy's 
doing with the StandaloneGlobalCollectAdapter. Or perhaps they could be part of 
a shared 'WMF fundraising' library. The library/libraries could then be used 
anywhere in the fundraising pipeline and you could be certain that you won't 
have feature drift - it will afford you /a lot/ of flexibility n the future. 
Duplicating this stuff in both DonationInterface and here will trip you up in 
the future - I guarantee it.  I realize that might complicate getting this done 
quickly, so perhaps now is not the right time to do it. But there might never 
be a 'good' time to do it. Just something to think about!

_______________________________________________
MediaWiki-CodeReview mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview

Reply via email to