dev/financial#82 revert added setters #15707
Conversation
|
(Standard links)
|
|
@mattwire @artfulrobot - so merging this is priority 1 in my opinion - then we can finalised @artfulrobot's improved version - but we should get this merged before the rc is cut |
There was a problem hiding this comment.
@eileenmcnaughton I think there's a few bits in here (mostly comments) that should not be.
| @@ -150,7 +139,7 @@ function civicrm_api3_payment_processor_pay($params) { | |||
| } | |||
| throw new API_Exception('Payment failed', $code, $errorData, $e); | |||
| } | |||
| return civicrm_api3_create_success([$result], $params); | |||
| return civicrm_api3_create_success(array($result), $params); | |||
There was a problem hiding this comment.
@eileenmcnaughton This shouldn't be in here
api/v3/PaymentProcessor.php
Outdated
| @@ -160,7 +149,7 @@ function civicrm_api3_payment_processor_pay($params) { | |||
| */ | |||
| function _civicrm_api3_payment_processor_pay_spec(&$params) { | |||
| $params['payment_processor_id'] = [ | |||
| 'api.required' => TRUE, | |||
| 'api.required' => 1, | |||
There was a problem hiding this comment.
@eileenmcnaughton This shouldn't be in here
| @@ -154,8 +143,6 @@ public function testPaymentProcessorDelete() { | |||
|
|
|||
| /** | |||
| * Check with valid params array. | |||
| * | |||
| * @throws \CRM_Core_Exception | |||
There was a problem hiding this comment.
@eileenmcnaughton None of this needs to go?
api/v3/PaymentProcessor.php
Outdated
| * @throws \CiviCRM_API3_Exception | ||
| */ | ||
| function civicrm_api3_payment_processor_pay($params) { | ||
| /* @var CRM_Core_Payment $processor */ |
There was a problem hiding this comment.
@eileenmcnaughton Don't need to remove?
CRM/Core/Payment.php
Outdated
| @@ -632,6 +257,8 @@ public function buildForm(&$form) { | |||
| * @todo move to factory class \Civi\Payment\System (or similar) | |||
| * | |||
| * @param array $params | |||
| * | |||
There was a problem hiding this comment.
@eileenmcnaughton This doesn't need removing
CRM/Core/Payment.php
Outdated
| * @var array | ||
| * @var object |
There was a problem hiding this comment.
@eileenmcnaughton Don't think this should be in here
eileenmcnaughton
force-pushed
the
setter
branch
from
e3bee45 to
03306ea
Compare
|
@mattwire I've re-applied some of those changes in an extra commit - I think most of them are easy enough to re-spot / re-do |
eileenmcnaughton
force-pushed
the
setter
branch
from
03306ea to
abf0d66
Compare
|
Thanks @mattwire - @artfulrobot - where are we at with your alternative - it's still marked WIP but I think you possibly think it is review-ready |
|
@eileenmcnaughton yes, I'll discuss that at #15697 (comment) |
Overview
We added some structure to CRM_Core_Payment in the current unreleased code. However @artfulrobot has a proposal that @mattwire & I both agree is better architecture and less likely to break stuff.
I think the cleanest option here is to simply reverse these changes in 5.20 and do them again, better. I'd like the better to be in 5.20 but if we missed 5.20 I think it would not cause issues
Before
New but now deprecated methods in play
After
Slate cleaned to try again
Technical Details
@mattwire @artfulrobot
Comments