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

Use already determined value for contributionRecurID #18265

Merged
merged 1 commit into from
Aug 27, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Aug 26, 2020

Overview

Just eliminates a place where $objects is referred to as we have already determined the value from $objects

Before

$contributionRecurID not used

After

$contributionRecurID used

Technical Details

I think we should move to passing in more $ids and less $objects. - Event & contribution are the only 2 objects currently where we are looking for more than just an id

Ie we would call it like

    CRM_Contribute_BAO_Contribution::completeOrder($input, [
      'related_contact' => $ids['related_contact'] ?? NULL,
      'participant' => isset($objects['participant']) ? $objects['participant']->id : NULL,
      'contributionRecur' => isset($objects['contributionRecur']) ? $objects['contributionRecur']->id : NULL,
    ], $objects);

The 2 remaining objects (event & contribution) we would probably load within completeOrder & not pass in $objects at all - but a little more work to confirm that. I think fixing batchUpdate to not call completeOrder (with tests) would simplify)

Comments

@civibot
Copy link

civibot bot commented Aug 26, 2020

(Standard links)

@civibot civibot bot added the master label Aug 26, 2020
@mattwire
Copy link
Contributor

Agree

@mattwire mattwire merged commit 58cfa1d into civicrm:master Aug 27, 2020
@eileenmcnaughton eileenmcnaughton deleted the objects branch August 27, 2020 09:14
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