r102084 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r102083‎ | r102084 | r102085 >
Date:04:37, 5 November 2011
Author:jpostlethwaite
Status:reverted (Comments)
Tags:fundraising 
Comment:
Changed getText() to getVal() where a null value was specified. Several of these just had the null second parameter removed because it was not necessary to be null.
Modified paths:
  • /trunk/extensions/DonationInterface/gateway_common/DonationData.php (modified) (history)

Diff [purge]

Index: trunk/extensions/DonationInterface/gateway_common/DonationData.php
——@@ -27,9 +27,9 @@
2828 $this->populateData_Test();
2929 } else {
3030 $this->normalized = array(
31 - 'amount' => $wgRequest->getText( 'amount', null ),
32 - 'amountGiven' => $wgRequest->getText( 'amountGiven', null ),
33 - 'amountOther' => $wgRequest->getText( 'amountOther', null ),
 31+ 'amount' => $wgRequest->getText( 'amount' ),
 32+ 'amountGiven' => $wgRequest->getText( 'amountGiven' ),
 33+ 'amountOther' => $wgRequest->getText( 'amountOther' ),
3434 'email' => $wgRequest->getText( 'emailAdd' ),
3535 'fname' => $wgRequest->getText( 'fname' ),
3636 'mname' => $wgRequest->getText( 'mname' ),
——@@ -59,14 +59,14 @@
6060 'cvv' => $wgRequest->getText( 'cvv' ),
6161 'currency' => $wgRequest->getVal( 'currency_code' ),
6262 'payment_method' => $wgRequest->getText( 'payment_method', 'cc' ),
63 - 'payment_submethod' => $wgRequest->getText( 'payment_submethod', null ), // Used by GlobalCollect for payment types
 63+ 'payment_submethod' => $wgRequest->getVal( 'payment_submethod' ), // Used by GlobalCollect for payment types
6464 'issuer_id' => $wgRequest->getText( 'issuer_id' ),
65 - 'order_id' => $wgRequest->getText( 'order_id', null ), //as far as I know, this won't actually ever pull anything back.
66 - 'i_order_id' => $wgRequest->getText( 'i_order_id', null ), //internal id for each contribution attempt
 65+ 'order_id' => $wgRequest->getVal( 'order_id' ), //as far as I know, this won't actually ever pull anything back. This does get pulled back on processing credit cards. 2011-11-04
 66+ 'i_order_id' => $wgRequest->getVal( 'i_order_id' ), //internal id for each contribution attempt
6767 'numAttempt' => $wgRequest->getVal( 'numAttempt', '0' ),
6868 'referrer' => ( $wgRequest->getVal( 'referrer' ) ) ? $wgRequest->getVal( 'referrer' ) : $wgRequest->getHeader( 'referer' ),
6969 'utm_source' => $wgRequest->getText( 'utm_source' ),
70 - 'utm_source_id' => $wgRequest->getVal( 'utm_source_id', null ),
 70+ 'utm_source_id' => $wgRequest->getVal( 'utm_source_id' ),
7171 'utm_medium' => $wgRequest->getText( 'utm_medium' ),
7272 'utm_campaign' => $wgRequest->getText( 'utm_campaign' ),
7373 // try to honor the user-set language (uselang), otherwise the language set in the URL (language)
——@@ -76,27 +76,27 @@
7777 'email-opt' => $wgRequest->getText( 'email-opt' ),
7878 // test_string has been disabled - may no longer be needed.
7979 //'test_string' => $wgRequest->getText( 'process' ), // for showing payflow string during testing
80 - '_cache_' => $wgRequest->getText( '_cache_', null ),
81 - 'token' => $wgRequest->getText( 'token', null ),
 80+ '_cache_' => $wgRequest->getVal( '_cache_' ),
 81+ 'token' => $wgRequest->getVal( 'token' ),
8282 'contribution_tracking_id' => $wgRequest->getText( 'contribution_tracking_id' ),
8383 'data_hash' => $wgRequest->getText( 'data_hash' ),
8484 'action' => $wgRequest->getText( 'action' ),
8585 'gateway' => $wgRequest->getText( 'gateway' ), //likely to be reset shortly by setGateway();
86 - 'owa_session' => $wgRequest->getText( 'owa_session', null ),
87 - 'owa_ref' => $wgRequest->getText( 'owa_ref', null ),
 86+ 'owa_session' => $wgRequest->getVal( 'owa_session' ),
 87+ 'owa_ref' => $wgRequest->getVal( 'owa_ref' ),
8888
89 - 'account_name' => $wgRequest->getText( 'account_name', null ),
90 - 'account_number' => $wgRequest->getText( 'account_number', null ),
91 - 'authorization_id' => $wgRequest->getText( 'authorization_id', null ),
92 - 'bank_check_digit' => $wgRequest->getText( 'bank_check_digit', null ),
93 - 'bank_name' => $wgRequest->getText( 'bank_name', null ),
94 - 'bank_code' => $wgRequest->getText( 'bank_code', null ),
95 - 'branch_code' => $wgRequest->getText( 'branch_code', null ),
96 - 'country_code_bank' => $wgRequest->getText( 'country_code_bank', null ),
97 - 'date_collect' => $wgRequest->getText( 'date_collect', null ),
98 - 'direct_debit_text' => $wgRequest->getText( 'direct_debit_text', null ),
99 - 'iban' => $wgRequest->getText( 'iban', null ),
100 - 'transaction_type' => $wgRequest->getText( 'transaction_type', null ),
 89+ 'account_name' => $wgRequest->getText( 'account_name' ),
 90+ 'account_number' => $wgRequest->getText( 'account_number' ),
 91+ 'authorization_id' => $wgRequest->getText( 'authorization_id' ),
 92+ 'bank_check_digit' => $wgRequest->getText( 'bank_check_digit' ),
 93+ 'bank_name' => $wgRequest->getText( 'bank_name' ),
 94+ 'bank_code' => $wgRequest->getText( 'bank_code' ),
 95+ 'branch_code' => $wgRequest->getText( 'branch_code' ),
 96+ 'country_code_bank' => $wgRequest->getText( 'country_code_bank' ),
 97+ 'date_collect' => $wgRequest->getText( 'date_collect' ),
 98+ 'direct_debit_text' => $wgRequest->getText( 'direct_debit_text' ),
 99+ 'iban' => $wgRequest->getText( 'iban' ),
 100+ 'transaction_type' => $wgRequest->getText( 'transaction_type' ),
101101 );
102102 if ( !$wgRequest->wasPosted() ) {
103103 $this->setVal( 'posted', false );

Follow-up revisions

RevisionCommit summaryAuthorDate
r102191Revert r102084awjrichards20:09, 6 November 2011

Comments

#Comment by Khorn (WMF) (talk | contribs)   04:55, 6 November 2011

All of the lines that didn't get changed to getVal, but had their default of 'null' removed, need to either be changed back, or changed to getVal like the others. getText's second parameter is not null: It is a blank string (See /includes/WebRequest.php).

#Comment by Jpostlethwaite (talk | contribs)   05:15, 6 November 2011

It was suggested we change them to reflect the proper usage of the function.

I added all of the direct debit with the null. I saw no reason to keep them as null, since they were using getText().

Is there a problem with them being an empty string? An empty string is what will be put in the form element, so it seems logical to deliver an empty string, instead of null.

#Comment by Khorn (WMF) (talk | contribs)   05:41, 6 November 2011

Well, for one instance, see getTransactionSpecificValue in the gateway.adapter (search the function for 'is_null'). This builds all our communication with the remote gateways, and the communication could vary drastically if it sees what it will identify as an explicitly sent blank string, and a piece of data that was missing or not sent at all.

True, all blank fields in a form will be an empty string. However, we do not know at the time of raw data population that this was even a form post from DonationInterface. There is a lot of processing to be done before we get to the point where we are populating form fields, and we need to be able to easily tell if any given value was explicitly passed in as an empty string, or if it was not passed in at all. Setting absent values to an empty string by default removes that ability.

#Comment by Jpostlethwaite (talk | contribs)   05:53, 6 November 2011

What about changing them all to getVal()?

They could be set in staging or wherever else is necessary.

#Comment by Jpostlethwaite (talk | contribs)   05:59, 6 November 2011

I ran into a problem earlier where I wanted to validate if state or country were set.

Because we are using getText(), it is always a string and not null.

#Comment by Awjrichards (talk | contribs)   20:10, 6 November 2011

I've reverted this revision as things were working fine before and not blocking anyone. After the FR launch, we can settle this debate and clean this stuff up.

Status & tagging log