All variable-table variables in this module (created by variable_set()) are prefixed "moneris_" rather than using the module name as a prefix. I suggest these prefixes be changed to "uc_moneris_".

If you think this is a good idea, I'll roll a patch for it after #502218: 6.x-1.x-dev branch does not contain Drupal 6 code is fixed.

CommentFileSizeAuthor
#3 502234_3_rename_variables_everywhere.patch13.89 KBgreggles
#2 uc_moneris.install.patch1.92 KBtr
#2 uc_moneris.module.patch7.81 KBtr

Comments

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

Yep, this makes sense to me.

tr’s picture

tr commented
Status: Active » Needs review
StatusFileSize
new uc_moneris.module.patch7.81 KB
new uc_moneris.install.patch1.92 KB

OK, here are the patches. uc_moneris.module.patch renames the variables in the module. uc_moneris.install.patch adds an update function which copies the values of the old variables into the new ones, then deletes the old ones.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
StatusFileSize
new 502234_3_rename_variables_everywhere.patch13.89 KB

Untested, but don't we need something more like the attached so that the form saves the variables in the right place? I changed the values in the form array in uc_moneris_settings_form.

tr’s picture

tr commented

I'm not against changing the form keys for consistency, but it's not necessary. The keys are only used to save variables when using system_settings_form(), which is not the case for this module.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Title: Rename variables » Rename variables to follow module name prefixing standard
Status: Needs review » Fixed

Ah, yeah, that makes sense. Thanks, TR!

Committed - http://drupal.org/cvs?commit=256168

tr’s picture

tr commented
Title: Rename variables to follow module name prefixing standard » Rename variables
Status: Fixed » Active

Actually ...

This got me thinking about how the form settings are saved in uc_moneris, so I looked at the code and I don't see anywhere where the variables are set - they're only used. So clearly they're saved somewhere else, outside the uc_moneris module.

Digging a little deeper, it seems uc_payment_gateways_form() invokes hook_payment_gateway() (by calling _payment_gateway_list()) in order to find out which functions the gateways use to define their settings. In the case of uc_moneris, that function would be uc_moneris_settings_form().

Then uc_payment_gateways_form() invokes these individual gateway settings forms and MERGES the returned $form from these functions into one big form, which is THEN processed as a system_settings_form() ! So that's how the form values get saved as variables - system_settings_form() IS being used, but this fact is obscured.

Bottom line, you're right that the form keys MUST be changed, but not for the reason you thought!

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Title: Rename variables » Rename variables to follow module name prefixing standard
Status: Active » Fixed

Crosspost? Or is there still work to do?

tr’s picture

tr commented
Status: Fixed » Active

Looks like you took care of renaming the form keys so this issue is fixed.

tr’s picture

tr commented
Status: Active » Fixed

Whoops, moving back to fixed.

j0rd’s picture

j0rd commented
Category: task » bug
Status: Fixed » Active

Aug 27th release contains:

api/mpgClasses.php

   $mode = variable_get('moneris_transaction_mode', 'test');
// should be this
// $mode = variable_get('uc_moneris_transaction_mode', 'test');

This makes all transactions getting sent out to the test server, which is causing me grief.

same issue in api/mpiClasses.php

j0rd’s picture

j0rd commented
Priority: Minor » Critical

changed to critical. Please make sure this is resolved in mpiClasses.php and mpgClasses.php

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

Now fixed http://drupal.org/cvs?commit=259240

I appreciate your help debugging this and pointing out the solution. Marking it critical doesn't actually make anyone want to fix things sooner. Much better would be to provide a patch - please see getting a bug fixed sooner.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Active » Fixed
j0rd’s picture

j0rd commented

THanks for the advice greggles. I figured a bug which makes your processor send all live transactions to the test server was best flagged as critical instead of minor. Had nothing to do with getting it fixed sooner as mine is already fixed :D As for the patch, my uc_moneris is a mess at the moment with debugging statements and such.

There are two other bug which I'm not sure if they're fixed.

One has to do with the improper naming of the txnType vs. txn_type variable. THe other has to do with sending an invalid transaction type when you turn off AVS or CVV checking.

Both issues are discussed here: http://drupal.org/node/525268

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.