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

Cleanup copyValues DAO function #16589

Merged
merged 1 commit into from
Feb 18, 2020
Merged

Conversation

colemanw
Copy link
Member

Overview

Cleans up function variables and removes a transitional parameter.

Before

  • The $serializeArrays parameter was transitional, but would trigger a test failure if it was incorrectly omitted. So at this point if all the tests are passing, it can be removed.
  • Unnecessary passing $params by reference.

After

  • The $serializeArrays parameter is gone as serialization is now always enabled.
  • Passing $params by reference was unnecessary and has been removed.
  • Renamed a few variables for readability.

The $serializeArrays parameter was transitional and is now always enabled.
Passing $params by reference was unnecessary and has been removed.
Renamed a few variables for readability.
@civibot
Copy link

civibot bot commented Feb 18, 2020

(Standard links)

@civibot civibot bot added the master label Feb 18, 2020
@colemanw
Copy link
Member Author

@eileenmcnaughton as we saw in #16575 the $serializeArrays param will cause noisy test failures if it is set to false when it should be true. But setting it to true doesn't hurt anything, so let's just get rid of it and treat it as always true.

@eileenmcnaughton
Copy link
Contributor

@colemanw sure - it was just caution & the fact we haven't been seeing those notices tells us there aren't other arrays going in

@eileenmcnaughton eileenmcnaughton merged commit 84c0256 into civicrm:master Feb 18, 2020
@eileenmcnaughton eileenmcnaughton deleted the copyValues branch February 18, 2020 19:47
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