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] Stop passing ids to membership::create from createRelatedMemberships #17087

Merged
merged 1 commit into from
Apr 17, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Optional param $ids should not be passed to Membership::create when it has no meaning

Before

CRM_Member_BAO_Membership::create($params, $ids);

After

CRM_Member_BAO_Membership::create($params);

Technical Details

We are passing in an empty array. Per the code comments there was concern that the array might NOT be empty after calling
create & that needed to be checked out. However, I just went through it & concluded that values in the ids var would
only ever be set if ids['membership'] was passed in - so if it goes in empty it will come out empty

Comments

@mattwire we stopped short of doing this last time but I dug a bit further & can't see how $ids would be altered in the function if it starts off empty

We are passing in an empty array. Per the code comments there was concern that the array might NOT be empty after calling
create & that needed to be checked out. However, I just went through it & concluded that values in the ids var would
only ever be set if ids['membership'] was passed in - so if it goes in empty it will come out empty
@civibot
Copy link

civibot bot commented Apr 16, 2020

(Standard links)

@civibot civibot bot added the master label Apr 16, 2020
@yashodha
Copy link
Contributor

yashodha commented Apr 17, 2020

@eileenmcnaughton looks good and seems to be in continuation of work in #17086

@mattwire mattwire merged commit e0dba58 into civicrm:master Apr 17, 2020
@eileenmcnaughton eileenmcnaughton deleted the ids branch April 20, 2020 23:08
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.

3 participants