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] [Import] Extract getContactType #23284

Merged
merged 1 commit into from
Apr 24, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Apr 22, 2022

Overview

[REF] [Import] Extract getContactType

Before

Repetitive Switch statement

After

Who woulda thunk it - re-usable function

Technical Details

The getSubmittedValue function is clever enough to retrieve this field as it was submitted on the DataSource form

Note that the function on the MetaDataTrait is moved to the
Parser_Class - 3 forms use the trait - a unittest class,
the Parser_Class and MapField.

MapField was not calling the old function but the newly added one
clashes - and highlights that the contents of the function
should differ for the 2 scenarios - hence moving
off the Trait

Comments

@civibot
Copy link

civibot bot commented Apr 22, 2022

(Standard links)

Note that the function on the MetaDataTrait is moved to the
Parser_Class - 3 forms use the trait - a unittest class,
the Parser_Class and MapField.

MapField was not calling the old function but the newly added one
clashes - and highlights that the contents of the function
should differ for the 2 scenarios - hence moving
off the Trait and onto the 3 classes that use it
@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy if you feel able to review this (or any of the other import ones) - we have a bit of an understanding with these that the individual PRs are getting lighter review / skipping r-run on some - because the changes are ongoing & the r-run keeps happening - in the case of this PR I have definitely r-run it myself

@colemanw has been doing a lot of heavy lifting on these - but he is travelling to Europe (yes!) over the weekend

* but it probably should be phased out again.
*/
protected function getContactType() {
return $this->_contactType ?? 'Individual';
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit misleading because $this is the test class not the form so the function always returns Individual, and it just happens to work out because the tests involved are testing Individual. So it's not that it should "probably" be phased out again, it does eventually need to be "fixed".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@demeritcowboy yeah - there was a reason for the trait when it was added (ie the code was so confusing that there needed to be a way to extract & clean up some parts) - but I think there are some better way to achieve the same things - hence it's the trait I want to phase out I did write some notes about the restructure & the goal https://lab.civicrm.org/dev/core/-/issues/3414 - but in terms of the trait I think the functions probably eventually belong to the Parser class - it's just that it has traditionally been so painful to call that class.

I've started to move towards things like this though https://github.com/civicrm/civicrm-core/pull/23288/files#diff-6c1a95e4ed243dbed9e0b7c980cd2fed4e538c09863b6ae45a2e0538d1db8321R668 (which is an example of accessing something from the MetadataTrait without using it

@demeritcowboy demeritcowboy added the merge ready PR will be merged after a few days if there are no objections label Apr 24, 2022
@demeritcowboy demeritcowboy merged commit 4d167cf into civicrm:master Apr 24, 2022
@eileenmcnaughton eileenmcnaughton deleted the import_contact_type branch April 24, 2022 19:28
@eileenmcnaughton
Copy link
Contributor Author

thanks @demeritcowboy

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
2 participants