Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REF] Duplicate & unshare processFormContribution #22276

Merged
merged 1 commit into from
Dec 20, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

[REF] Duplicate & unshare processFormContribution

Before

Code shared between front end & back office forms - but not in a way that reduces the total amount of code

After

Code duplicated & unshared - allowing further cleanup

Technical Details

The only way I've managed to break up these big toxic functions is divide & conquer. While it seems
counter-intuitive to not doing things in more than one place in practice much of the
code is form-relevant and the the code before, during and after these functions
does a whole lot of extra work to share stuff that they don't really have in common.

This dates back pre-api when the only copy of the business logic often was on the forms....

Comments

The only way I've managed to break up these big toxic functions is divide & conquer. While it seems
counter-intuitive to not doing things in more than one place in practice much of the
code is form-relevant and the the code before, during and after these functions
does a whole lot of extra work to share stuff that they don't really have in common.

This dates back pre-api when the only copy of the business logic often was on the forms....
@civibot
Copy link

civibot bot commented Dec 19, 2021

(Standard links)

@civibot civibot bot added the master label Dec 19, 2021
@@ -1043,7 +1046,7 @@ protected function postProcessPremium($premiumParams, $contribution) {
* @throws \CRM_Core_Exception
* @throws \CiviCRM_API3_Exception
*/
public static function processFormContribution(
protected static function processFormContribution(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obviously it's not supported to call this from outside core - but I did do a universe search to double check

@colemanw
Copy link
Member

Yay, more crappy code. Thanks @eileenmcnaughton

@colemanw colemanw merged commit eedf2b1 into civicrm:master Dec 20, 2021
@colemanw colemanw deleted the process branch December 20, 2021 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants