"Awjrichards" posted a comment on Wikimedia.r1212.
URL: http://www.mediawiki.org/wiki/Special:Code/Wikimedia/1212#c30128
Commit summary for Wikimedia.r1212:
Adding module for Global Collect recurring payments.
Awjrichards's comment:
Unless you are planning to add some magic to
trunk/fundraising-civicrm/sites/all/modules/civicrm/CRM/Core/Payment/GlobalCollect.php
and push it upstream to CiviCRM, you should probably not put this here - there
is a chance that this will goober up a future CiviCRM upgrade. In general, do
not modify CiviCRM core unless you are planning to push it upstream and
document it heavily for the next guy :)
In, civicrm_recurring_globalcollect.info, I recommend not packaging this module
with 'CiviCRM'. In the long term, we might consider better organizing our
modules into a WMF package or something, but for now, this might just stand on
its own. If you leave in the 'CiviCRM' picture, it might be construed as
actually being a part of core CiviCRM, which it's not.
In civicrm_reccuring_globalcollect.module:
<pre>
require_once( drupal_get_path( 'module', 'civicrm_recurring_globalcollect' ) .
'/library/StandaloneGlobalCollectAdapter/StandaloneGlobalCollectAdapter.php' );
</pre>
>From looking at the codebase in general, I think it's unlikely that we'll put
>the standalone adapter class in the module, and it will be an administrative
>pain to keep it symlinked. One thing I recommend you do instead is make the
>path to the adapater configurable in an administrative interface. Look in
>queue2civicrm for examples.
This is not necessarily a big deal, but doesn't the adapter have facilities for
checking IP address? It might be nice to stay consistent and use what's
provided in the adapter.
<pre>
$instance = StandaloneGlobalCollectAdapter::getInstance( $options );
$IPADDRESS = isset($result['$IPADDRESS']) ? $result['$IPADDRESS'] : '';
$IPADDRESS = (empty($IPADDRESS) && isset($_SERVER['SERVER_ADDR'])) ?
$_SERVER['SERVER_ADDR'] : '';
if (empty($IPADDRESS)) {
$IPADDRESS = '127.0.0.1';
$message = 'You must specify a valid IPA.';
//throw new Exception($message);
}
</pre>
And re:
<pre>
/**
* civicrm_recurring_globalcollect_mail
*
* Is this being called somewhere? Is it a hook?
*
* @param type $key
* @param type $message
* @param type $params
*/
</pre>
Yes, it is a hook. See
http://api.drupal.org/api/drupal/includes--mail.inc/function/drupal_mail/6 for
more details.
Something you might consider is relying on the escaping built into the
db_query() command
(http://api.drupal.org/api/drupal/includes--database.mysql-common.inc/function/db_query/6).
This will help you stay consistent with Drupal style and also will ensure
proper escaping of arguments for various possible database engines.
It seems that you are setting things up in the module to be easily testable,
which is really cool. You should take a look at 'simpletest' (we already have
it set up in repo), which is a Drupal module for functional (not exactly
unit-based) testing. You can see examples in queue2civicrm module and talk to
Katie or myself about making it go. It also integrates nicely with Drush.
As a general style comment, I believe for both Drupal and Mediawiki the
preference is to keep line lengths to 80 chars. A lot of your lines go well
beyond that, which can make editing/viewing difficult in some instances,
particularly in a terminal. Not a huge deal, just something to think about.
_______________________________________________
MediaWiki-CodeReview mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
