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

Fix \Civi\Token\TokenRow::customToken() failure if field is not set #13280

Merged
merged 1 commit into from
Jan 3, 2019

Conversation

aydun
Copy link
Contributor

@aydun aydun commented Dec 14, 2018

Overview

Extracted from #13174 and #12012
Fixes a problem in \Civi\Token\TokenRow::customToken(). If the field is not set or does not exist, the getValue API call blows up, so use

Before

Calling \Civi\Token\TokenRow::customToken() with a custom field that is not set fails

After

Calling \Civi\Token\TokenRow::customToken() with a custom field that is not set works

@civibot civibot bot added the master label Dec 14, 2018
@civibot
Copy link

civibot bot commented Dec 14, 2018

(Standard links)

@mattwire
Copy link
Contributor

mattwire commented Jan 3, 2019

getvalue will fail if the (custom) field does not exist. For tokens we don't want to abort, just skip that token and continue. So this is doing the right thing by switching to getsingle.

@aydun Are you sure that all entity APIs will return the custom fields without specifying them in the return? getsingle won't fail if the customfield name is specified in the return but does not exist.

@aydun
Copy link
Contributor Author

aydun commented Jan 3, 2019

@mattwire Many thanks for reviewing all these changes. Not sure I understand your comment on this one since it does specify the custom field in return. I don't know whether all entities would return the custom fields without specifying them but it seems safer and more performant to specify it. If the field is not returned by getsingle for any reason then $fieldValue is set to the empty string.

@mattwire
Copy link
Contributor

mattwire commented Jan 3, 2019

@aydun. Yes I'm sorry! I missed the 'return' => line... So my suggestion was that you add what is already there to make sure it works...

@eileenmcnaughton @seamuslee001 This is ok to merge from my perspective.

@aydun
Copy link
Contributor Author

aydun commented Jan 3, 2019

@mattwire Thanks!

@seamuslee001
Copy link
Contributor

Merging as per review from Matt

@seamuslee001 seamuslee001 merged commit 44e82e1 into civicrm:master Jan 3, 2019
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.

3 participants