"Awjrichards" changed the status of Wikimedia.r1392 to "fixme" and commented it.
URL: http://www.mediawiki.org/wiki/Special:Code/Wikimedia/1392#c31873
Old Status: new
New Status: fixme
Commit summary for Wikimedia.r1392:
This should resolve the last issue on r1212.
_civicrm_recurring_globalcollect_update_record() was broken up into smaller
parts, which will make it easier to see what is going on with prepping the
fields to be updated.
Awjrichards's comment:
IMHO, this is even more complicated and difficult to review than what was in
place before.
Doing this:
<pre>
function _civicrm_recurring_globalcollect_data_set_amount(&$data) {
$return = '';
// Set amount if present
if (isset($data['amount'])) {
if (!is_float($data['amount'])) {
$message = 'Amount must be a float value';
throw new Exception($message);
}
$return = " `amount` = '" . db_escape_string($data['amount']). "'";
}
return $return;
}
</pre>
Followed by this:
<pre>
if ( !empty($amount) ) {
$set .= $amount;
}
</pre>
seems like a lot, potentially error-prone, difficult to test, difficult to
review (particularly for security concerns), etc.
I think part of the problem here is that you're conflating data validation/data
checking with query construction. I suggest logically separating those tasks
_______________________________________________
MediaWiki-CodeReview mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
