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] Fix CRM_Core_BAO_UFGroup::createUFJoin to not receive by reference. #16260

Merged
merged 1 commit into from
Jan 22, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Minor code cleanup - stop passing variable by reference, pass actually required values

Before

public static function createUFJoin(&$params, $ufGroupId) {

After

public static function createUFJoin($params, $ufGroupId) {

In addition a small array containing only the 2 params used by the function is passed to it, getting away from 're-using' $params

Technical Details

This function is called from one place. It gets passed a params variable that is 're-used' so this
change makes it clear what actually needs to be passed on. We have an anti-pattern in our code whereby params get
passed all sorts of places with it being confusing which ones actually need to receive what

Comments

@civibot
Copy link

civibot bot commented Jan 9, 2020

(Standard links)

@civibot civibot bot added the master label Jan 9, 2020
@eileenmcnaughton
Copy link
Contributor Author

test this please

@colemanw
Copy link
Member

colemanw commented Jan 11, 2020

If only 2 variables are needed, I would vote for having them be function params, e.g.

CRM_Core_BAO_UFGroup::createUFJoin($ufGroupId, $ufGroupType, $weight)

…nce.

This function is called from one place. It gets passed a params variable that is 're-used' so this
change makes it clear what actually needs to be passed on. We have an anti-pattern in our code whereby params get
passed all sorts of places with it being confusing which ones actually need to receive what
@eileenmcnaughton
Copy link
Contributor Author

@colemanw fixed

@eileenmcnaughton
Copy link
Contributor Author

@colemanw see above for changes you suggested

@jitendrapurohit
Copy link
Contributor

I've just tested this manually and looks fine to me. Confirmed that the function is called only from CRM/UF/Form/Group.php and the new parameters work fine in storing the values to the database. +1 to merge this change.

@eileenmcnaughton eileenmcnaughton merged commit b05b03e into civicrm:master Jan 22, 2020
@eileenmcnaughton
Copy link
Contributor Author

Thanks @jitendrapurohit

@eileenmcnaughton eileenmcnaughton deleted the ids_2 branch January 22, 2020 18:48
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