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] Extract getSearchSQL from getSearchQuery. #13668

Merged
merged 1 commit into from
Feb 25, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Code cleanup - function extraction

Before

Less readable

After

More readable

Technical Details

I made 2 changes over & above the function extraction

  1. I changed one place in the code to call the new version - mostly for show - will do more later
  2. I made it so the re-enable is called regardless of whether it was disabled after running the query. My logic is that it will only re-enabled IF it is enabled for the site so we should be safe. Doing this simplifies the code

Comments

@civibot
Copy link

civibot bot commented Feb 22, 2019

(Standard links)

@civibot civibot bot added the master label Feb 22, 2019
CRM_Core_DAO::reenableFullGroupByMode();
}
// We can always call this - it will only re-enable if it was originally enabled.
CRM_Core_DAO::reenableFullGroupByMode();
Copy link
Contributor Author

@eileenmcnaughton eileenmcnaughton Feb 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the re-enable function is safe as it re-enables if previously enabled but I won't enable if never enabled

Change one instance to call it directly. Next I'll deprecate it & change all instances hit by the tests
@eileenmcnaughton eileenmcnaughton force-pushed the extract_order branch 2 times, most recently from 832d1d3 to 89d7bb7 Compare February 24, 2019 21:26
@colemanw
Copy link
Member

@eileenmcnaughton is this covered by tests?

@eileenmcnaughton
Copy link
Contributor Author

@colemanw yep - this function is heavily tested

@colemanw
Copy link
Member

Merging based on code review and passing tests.

@colemanw colemanw merged commit 4bbf7b9 into civicrm:master Feb 25, 2019
@colemanw colemanw deleted the extract_order branch February 25, 2019 02:18
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