User "Awjrichards" changed the status of Wikimedia.r198. Old Status: new New Status: fixme
User "Awjrichards" also posted a comment on Wikimedia.r198. Full URL: http://www.mediawiki.org/wiki/Special:Code/Wikimedia/198#c19052 Commit summary: Fixing my unit test, adding a config.ini-dist, and fixing the thing it was testing as well. Story 149 in the 2011 fundraiser. Comment: I see these getting used in a few places in trunk/fundraising-misc/queue_handling/payflowpro/tests/StompPFPPendingProcessorSATest.php: <pre> + foreach ($logdata as $line_no => $line_data) { + if (strpos($line_data, "Handling transaction ") > 0) { + $resultLines[$line_no] = $line_data; + } + } + + //at this point, the key and value of each $transactions is the same. + foreach ($transactions as $id => $transaction) { + foreach ($resultLines as $line_no => $line_data) { + if (strpos($line_data, $id) > 0) { + $transactions[$id] = $line_no; + } + break; + } + if (array_key_exists($transactions[$id], $resultLines)) { + unset($resultLines[$transactions[$id]]); + } + } </pre> You might consider functionizing these for cleaner reuse. <pre> + $payflow_query = implode('&', $query); </pre> This should probably be wrapped in rawurlencode() or something. <pre> + if(array_key_exists('HTTP_USER_AGENT', $_SERVER)){ + $user_agent = $_SERVER['HTTP_USER_AGENT']; + } else { + $user_agent = "blargh?"; + } </pre> Heh - the 'else' statement should perhaps set the user agent to something more clear/obvious as to what's going on - in this case, if no user agent exists in the $_SERVER var we're probably runing this from the cli and/or as a test, so perhaps a useful user agent would identify the script/owner/version/whatever (eg 'WMF PayflowPro Pending Processor') in the handle_pending_transaction() method of the processing class, why do we pass it an expclicitly json encoded object only to decode immediately in the method? This might be something silly I initially wrote in the class... is this totally necessary? there is an exit(1) in fetch_payflow_transaction_status() - we should probably have this method return false and have the executing script handle exit stuff like you did in set_stomp_connection(). _______________________________________________ MediaWiki-CodeReview mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
