| Subject: | _child_submit/_pre_submit() wackiness causes deep recursion in BOP::AuthorizeNet |
The odd code to insert the new _pre_submit() into a subclass' submit()
override causes BOP::AuthorizeNet's capture.t test to go into deep
recursion.
The first object created will have _child_submit set to its submit()
method. The subclass' submit method will then be overriden by BOP.
Everything works fine. But the second object created will have
_child_submit set to the BOP override. This is called inside the BOP
override and deep recursion happens.
Rather than that, the attached patch simply overrides the subclass'
submit and records that we've done it so we don't do it again. The
submit override calls both pre_submit() and then the original submit.
Overall this implementation of wedging fraud detection in seems fragile
and is setting off all sorts of design alarm bells. The necessity of it
comes from subclasses not properly calling SUPER::submit() in their
submit() overrides (rightly, because it would blow up) leaving no way
for BOP to add more functionality to submit().
Rather than hack things up like this it might be better to just change
the API and require folks to call SUPER::method when the override BOP
methods. In this case it does no harm to existing code except they
can't use fraud protection until they update. Send a note out to the
BOP subclass authors about the change.
| Subject: | bop_pre_submit.patch |
=== OnlinePayment.pm
==================================================================
--- OnlinePayment.pm (revision 19627)
+++ OnlinePayment.pm (local)
@@ -12,6 +12,8 @@
my $Class = __PACKAGE__;
+my %Presubmit_Added;
+
my %fields = (
authorization => undef,
error_message => undef,
@@ -56,15 +58,16 @@
$self->$key($value);
}
- unless ( $subclass->can('submit') eq $class->can('submit') ) {
+ unless ( $Presubmit_Added{$subclass}++ ) {
no strict 'refs';
no warnings 'redefine';
- my $submit = qualify_to_ref('submit', $subclass);
- $self->{_child_submit} = \&$submit;
+ my $real_submit = $subclass->can('submit');
*{"${subclass}::submit"} = sub {
my $self = shift;
- $self->_pre_submit();
+ return unless $self->_pre_submit(@_);
+
+ return $real_submit->($self, @_);
}
}
@@ -80,7 +83,7 @@
$risk_transaction->submit();
if ($risk_transaction->is_success()) {
if ( $risk_transaction->fraud_score <= $self->maximum_fraud_score()) {
- $self->{_child_submit}->($self);
+ return 1;
} else {
$self->is_success(0);
$self->error_message('Excessive risk from risk management');
@@ -96,7 +99,7 @@
my $fraud_detection = $self->fraud_detect();
# early return if user does not want optional risk mgt
- return $self->{_child_submit}->($self,@_) unless $fraud_detection && length $fraud_detection;
+ return unless $fraud_detection && length $fraud_detection;
# Search for an appropriate FD module
foreach my $subclass ( q(Business::OnlinePayment::) . $fraud_detection,
=== t/pre_submit.t
==================================================================
--- t/pre_submit.t (revision 19627)
+++ t/pre_submit.t (local)
@@ -0,0 +1,33 @@
+#!/usr/bin/perl -w
+
+use Test::More tests => 4;
+
+my $pre_submit_called = 0;
+{ # fake test driver (with submit method)
+
+ package Business::OnlinePayment::MOCK;
+ use strict;
+ use warnings;
+ use base qw(Business::OnlinePayment);
+ sub submit {
+ my $self = shift;
+ return 1;
+ }
+
+ sub _pre_submit {
+ my $self = shift;
+ $pre_submit_called++;
+
+ return 1;
+ }
+}
+
+$INC{"Business/OnlinePayment/MOCK.pm"} = "testing";
+
+my $tx = Business::OnlinePayment->new("MOCK");
+ok $tx->submit;
+is $pre_submit_called, 1;
+
+my $tx2 = Business::OnlinePayment->new("MOCK");
+ok $tx2->submit;
+is $pre_submit_called, 2;