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

Remove unused variables in completeOrder() #15841

Conversation

mattwire
Copy link
Contributor

Overview

Pre for #15555 This removes unused variables from completeOrder() function.

Before

Unused variables being set in completeOrder() - more difficult to see where vars are actually being used.

After

Not set - easier to see where vars are actually being used.

Technical Details

Comments

Follow up to #15699

@civibot
Copy link

civibot bot commented Nov 13, 2019

(Standard links)

@demeritcowboy
Copy link
Contributor

demeritcowboy commented Nov 19, 2019

Looks ok except for maybe one possibility which I'm not sure about because I'm not familiar with this area of code.

The previous elseif (!empty($contribution->_relatedObjects['membership'])) { clause would never run if there were participant related objects. Now the if will always check against membership because of the removal of if (!empty($contribution->_relatedObjects['participant'])) {.

The PR title doesn't say NFC but it seems like it was intended to be, so to be truly NFC the new if line would need to be if (empty($contribution->_relatedObjects['participant']) && !empty($contribution->_relatedObjects['membership'])) {

Otherwise though $input isn't passed by reference, and it's clear the $input member elements referenced here aren't used again in the rest of the function.

@mattwire mattwire force-pushed the participant_cleanup_removeparticipantfrominput branch from 4cb4eda to 251a4af Compare November 19, 2019 02:42
@mattwire
Copy link
Contributor Author

@demeritcowboy Thanks for the review, I'm pretty sure it won't make a difference, but I've made the change you suggested as my priority is to remove the $participant->id from here

@demeritcowboy
Copy link
Contributor

Cool. And the fixme comment is perfect - it's exactly the part I was unsure about - whether it could actually be removed completely. So looks good to me.

@mattwire mattwire added the merge ready PR will be merged after a few days if there are no objections label Nov 19, 2019
@eileenmcnaughton
Copy link
Contributor

OK - yes I agree - we established previously that input does not need to be passed by reference & now that it is not we don't need to alter $input in this function. Merging

@eileenmcnaughton eileenmcnaughton merged commit eda4c59 into civicrm:master Nov 21, 2019
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Nov 21, 2019
Per @mattwiree digging here civicrm#15841
we don't need to set  vars that are no longer used
@eileenmcnaughton
Copy link
Contributor

Looking into this I added a PR to remove a few more lines #15901

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants