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

APIv4 - Fix output of CustomValue create/save/update #18195

Merged
merged 1 commit into from
Aug 20, 2020

Conversation

colemanw
Copy link
Member

Overview

Fixes the APIv4 output when saving custom pseudo-entities

Before

Output was the fairly useless [is_error => 0, result => true]

After

Outputs the actual values of the data saved, including the id. This is standard across other entities in the API.

Technical Details

In doing this I updated the internal CRM_Core_BAO_CustomValueTable::setValues() function to throw exceptions instead of returning errors, as that's been the goal for quite a while. We'll see what the tests think.

@civibot
Copy link

civibot bot commented Aug 19, 2020

(Standard links)

@civibot civibot bot added the master label Aug 19, 2020
@seamuslee001
Copy link
Contributor

Well tests have passed here @colemanw is this mergable?

@colemanw
Copy link
Member Author

@eileenmcnaughton do you feel good about switching to throw here?

@eileenmcnaughton
Copy link
Contributor

@colemanw yes- but we throw CRM_Core_Exception not just Exception. You could argue the reason but it's our current convention & we should argue the reason in gitlab before changing

@colemanw
Copy link
Member Author

Ok, FYI the reason I did that was because it was calling CRM_Core_Error::createAPIError which does throw Exception. But I agree that's better. Will rebase.

Before: output contained no useful data
After: output contains values and id
@eileenmcnaughton
Copy link
Contributor

Looks like we are close to fully removing createAPIError

@seamuslee001 seamuslee001 merged commit 624a006 into civicrm:master Aug 20, 2020
@seamuslee001 seamuslee001 deleted the customValueSave branch August 20, 2020 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants