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] Deprecate BAO_Contact::retrieve #22966

Merged
merged 1 commit into from
Mar 25, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Mar 18, 2022

Overview

This removes 2 calls to BAO_Contact::retrieve and marks it as deprecated.

It is a surprisingly complex function and all the calls to it only use parts of what it
does - in general just having the code 'do the thing' is better.

In this case it turns out there is no need for the contact objects. There is
also some pretty funky handling for the location keys in the next section
so making it clearer what is in them should make it possible to simplify that....

Before

BAO_Contact::retrieve called - but with a bunch of pre-work to control the output

After

The functions actually called moved in

Technical Details

I have a follow up that builds on this

Comments

This removes 2 calls to BAO_Contact::retrieve and marks it as deprecated.

It is a surprisingly complex function and all the calls to it only use parts of what it
does - in general just having the code 'do the thing' is better.

In this case it turns out there is no need for the contact objects. There is
also some pretty funky handling for the location keys in the next section
so making it clearer what is in them should make it possible to simplify that....
@civibot
Copy link

civibot bot commented Mar 18, 2022

(Standard links)

@civibot civibot bot added the master label Mar 18, 2022
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Mar 18, 2022
This builds on civicrm#22966 to improve loading of location entities.

It addresses 2 problems
1) the code is really confusing - handling the loading in 2 places
2) not all keys are loaded - resulting in enotices at the tpl layer.

Note that address still seems kinda tricky so I haven't worked through
that in this PR.
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Mar 18, 2022
This builds on civicrm#22966 to improve loading of location entities.

It addresses 2 problems
1) the code is really confusing - handling the loading in 2 places
2) not all keys are loaded - resulting in enotices at the tpl layer.

Note that address still seems kinda tricky so I haven't worked through
that in this PR.
@eileenmcnaughton
Copy link
Contributor Author

@colemanw can I get a merge on this - #22967 builds on it to fix a particularly annoying set of notices

@colemanw
Copy link
Member

Ok. Refactor is the same before/after just in a different spot.

@colemanw colemanw merged commit 2b2ecd5 into civicrm:master Mar 25, 2022
@colemanw colemanw deleted the retrieve branch March 25, 2022 13:33
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Mar 25, 2022
This builds on civicrm#22966 to improve loading of location entities.

It addresses 2 problems
1) the code is really confusing - handling the loading in 2 places
2) not all keys are loaded - resulting in enotices at the tpl layer.

Note that address still seems kinda tricky so I haven't worked through
that in this PR.
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Mar 31, 2022
This builds on civicrm#22966 to improve loading of location entities.

It addresses 2 problems
1) the code is really confusing - handling the loading in 2 places
2) not all keys are loaded - resulting in enotices at the tpl layer.

Note that address still seems kinda tricky so I haven't worked through
that in this PR.
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Apr 1, 2022
This builds on civicrm#22966 to improve loading of location entities.

It addresses 2 problems
1) the code is really confusing - handling the loading in 2 places
2) not all keys are loaded - resulting in enotices at the tpl layer.

Note that address still seems kinda tricky so I haven't worked through
that in this PR.
monishdeb pushed a commit to JMAConsulting/civicrm-core that referenced this pull request Apr 14, 2022
This builds on civicrm#22966 to improve loading of location entities.

It addresses 2 problems
1) the code is really confusing - handling the loading in 2 places
2) not all keys are loaded - resulting in enotices at the tpl layer.

Note that address still seems kinda tricky so I haven't worked through
that in this PR.
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