CRM-20625 Don't crash if we can't find the payment processor class. #11631
Conversation
Civi/Payment/System.php
Outdated
| throw new \CRM_Core_Exception('no class provided'); | ||
| } | ||
| require_once str_replace('_', DIRECTORY_SEPARATOR, $paymentClass) . '.php'; | ||
| if (class_exists($paymentClass)) { | ||
| require_once str_replace('_', DIRECTORY_SEPARATOR, $paymentClass) . '.php'; |
There was a problem hiding this comment.
I wonder if we still need this?
There was a problem hiding this comment.
actually by definition we don't - ie. class_exists will fail if not already included
CRM/Admin/Form/PaymentProcessor.php
Outdated
| /** | ||
| * @var int $_ppType Payment processor Type ID | ||
| */ | ||
| protected $_ppType; |
There was a problem hiding this comment.
Can we keep away from vars that are not readable - ie. ppType would be more readable as paymentProcessorType or processorType
CRM/Admin/Form/PaymentProcessor.php
Outdated
| @@ -332,9 +342,13 @@ public function setDefaultValues() { | |||
|
|
|||
| CRM_Core_DAO::storeValues($dao, $defaults); | |||
| // When user changes payment processor type, it is passed in via $this->_ppType so update defaults array. | |||
| if ($this->_ppType) { | |||
| if ($this->_ppType && array_key_exists($this->_ppType, $this->_ppTypes)) { | |||
There was a problem hiding this comment.
putting ppTypes in a static var is not actually gaining any efficiency over calling CRM_Core_PseudoConstant::paymentProcessorType() since it is already cached
|
@mattwire this makes a lot of sense - I made a few trivial comments but the thing I'm thinking is whether |
|
@eileenmcnaughton Thanks for the review. I agree with your proposed changes and have made them in a separate commit as they would obscure the original fixes and be difficult to review. |
|
@mattwire that looks fine but can we squash them before merging. The commit history outlives the review substantially so I'd rather not introduce a parameter & then rename it in a separate commit. FWIW my feeling is that commits should be logical & discrete & not show the 'workings' of getting to the final patch for the sake of people who come later. But I think multiple commits in a PR are fine as long as they are logical & discrete & not reworkings |
… when editing a payment processor that doesn't exist.
mattwire
force-pushed
the
CRM-20625_payproc_extension_dont_kill_civi
branch
from
2e03c7b to
8461467
Compare
|
@eileenmcnaughton Squashed :-) |
|
Great let's get this into the rc since someone is hurting for it on chat I believe - merge on pass |
Overview
If the payment processor doesn't exist CiviCRM crashes horribly and you can't do anything (via the UI). This can happen for a couple of reasons:
Before
CiviCRM crashes and you can't restore the extension (except via CLI wizardry).
After
CiviCRM does not crash.
Any payment processor with missing class is not loaded (the same as if it had an invalid config).
If you edit payment processors you get a warning message if the payment processor no longer exists and you can change it to another type if desired.
Technical Details
CiviCRM was not checking for payment processor classes before trying to use them.
Also identified a bug where the first time you save after changing a payment processor, the values of the previous payment processor are still saved - we need to reload the payment processor object before saving if the type has changed.