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 - make condition clearer #23382

Merged
merged 1 commit into from
May 9, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented May 5, 2022

Overview

[REF] Import - make condition clearer

Before

The check for whether we are dealing with a relationship mapping relies on the implication that $fieldName is only set if not a relationship (which relies on the initiating function doing some work)

After

We explicitly if-else $relatedContactKey

Technical Details

This has some whitespace so use w=1

When the class is instantiated the class that constructs is specifies the mapped field names - it leaves it blank when the mapped field is a relationship - making $fieldName an implicity proxy for 'is a relationship' - but it's a lot easier to read when we can see 'here is what the code does if it is a relationship & here is what it does otherwise

Comments

The variable is NULL or something like '5_a_b' - so isset not required

@civibot
Copy link

civibot bot commented May 5, 2022

(Standard links)

@civibot civibot bot added the master label May 5, 2022
The mapping of fields basically breaks down into 'has a relationship' - in which
case fieldName is empy and does not have a relationship - in which
case field name is not empy. This makes that explicit -
specifically referring to relatedContactKey rather than
relying on the person reading the code
understanding that the presence of fieldName implies
whether it is a relationship
@colemanw colemanw merged commit 3cfe4fd into civicrm:master May 9, 2022
@colemanw colemanw deleted the import_simp branch May 9, 2022 17:45
@colemanw
Copy link
Member

colemanw commented May 9, 2022

Makes sense

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