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 - don't throw exception when updating/deleting 0 items #16374

Merged
merged 2 commits into from
Jan 31, 2020

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Jan 24, 2020

Overview

When 0 items are found to be processed by batch actions, it's not really an error so we shouldn't throw an exception.

Before

*UpdateAction and DAODeleteAction throw exceptions when 0 items found.

After

No exception, just returns an empty result.

@civibot
Copy link

civibot bot commented Jan 24, 2020

(Standard links)

@civibot civibot bot added the master label Jan 24, 2020
@seamuslee001
Copy link
Contributor

test fails relate here @colemanw

@colemanw colemanw force-pushed the api4Delete branch 2 times, most recently from 63941dd to ff8584d Compare January 27, 2020 15:36
@alifrumin
Copy link
Contributor

I tested this and it works as described and seems sensible.

BEFORE this change:

Attempting a CiviCRM API4 delete call with no records found resulted in an error message like "Cannot delete Activity, no records found with subject = 1111"

WITH this change it results in an empty array.

@eileenmcnaughton
Copy link
Contributor

@colemanw this is a philosophical difference to apiv3 - being able to do things differently is obviously part of the point of apiv4 - but is it documented clearly as a difference?

@artfulrobot
Copy link
Contributor

As someone new to API v4, I have to admit I see it more like a SQL query builder than an API like V3.

As such I was surprised that a delete call (which allows multiple deletions now, like SQL) threw an exception if there was nothing to delete.

So I think this change is sensible.

Api3 was/is full of inconsistencies around how records can be specified, so I think this pr makes apiv4 more consistent and easier to document.

@eileenmcnaughton
Copy link
Contributor

I'm not opposed I just think this is a 'needs documentation'

@colemanw
Copy link
Member Author

I started to add docs to this then realized that Update currently works the same way. It throws an exception if 0 records are found to update. That also feels wrong to me (as in, not what SQL does), so I'd like to change that too.
Any objections?

@artfulrobot
Copy link
Contributor

Not from me. It seems correct on something that takes search parameters that it should deal with whatever results there are or aren't.

@colemanw colemanw changed the title APIv4 - don't throw exception when deleting 0 items APIv4 - don't throw exception when updating/deleting 0 items Jan 30, 2020
@colemanw
Copy link
Member Author

colemanw commented Jan 30, 2020

Update: I've updated this PR to update Update and also updated the docs 😆

civicrm/civicrm-dev-docs@f0232d2

@colemanw colemanw merged commit fd4aac2 into civicrm:master Jan 31, 2020
@colemanw colemanw deleted the api4Delete branch January 31, 2020 00:23
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.

5 participants