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 mandatory keys check fail for value of float 0.0 in a required key in an entity #20342

Merged
merged 2 commits into from
May 20, 2021

Conversation

bugfolder
Copy link
Contributor

Overview

Fixes https://lab.civicrm.org/dev/core/-/issues/2071.

The mandatory keys check in function civicrm_api3_verify_mandatory() fails erroneously if the key exists but it has the floating-point value of 0.0.

Reproduction steps

Using APIv3, create an entity that contains a required key with the floating-point value of 0.0, for example, line_total in a price field element.

Before

APIv3 throws a Mandatory key(s) missing from params array: exception.

After

Successful creation.

Technical Details

In civicrm/api/v3/utils.php, change line 92 from

if (!array_key_exists($key, $params) || (empty($params[$key]) && $params[$key] !== 0 && $params[$key] !== '0')) {

to

if (!array_key_exists($key, $params) || (empty($params[$key]) && $params[$key] !== 0.0 && $params[$key] !== '0')) {

The second version will match on both integer 0 and float 0.0, while the first only matches on integer 0 but not float 0.0.

@civicrm-builder
Copy link

Can one of the admins verify this patch?

@civibot
Copy link

civibot bot commented May 18, 2021

(Standard links)

@civibot civibot bot added the master label May 18, 2021
@eileenmcnaughton
Copy link
Contributor

add to whitelist

@eileenmcnaughton
Copy link
Contributor

@bugfolder - would you feel comfortable adding a unit test for this? I can help you if you want but as it's your first contribution you get to opt out of doing a test this time if it's too big a lift.

However, there is one thing you'll need to do once this is merged - add a PR like this ee8957e - erm but with your details , not Sunil's :-)

@bugfolder
Copy link
Contributor Author

Looking at the Developer guide for testing, it looks like a pretty big chunk of time and effort to get set up to write and debug unit tests. So maybe I'll wait till I make a PR that's bigger than two characters. ;o) But I will watch for the merge notice and add a PR with the Contributor changes.

@eileenmcnaughton
Copy link
Contributor

@bugfolder looks like you triggered a tonne of test fails - I think you want to add !== 0.0 rather than replace !== 0

@bugfolder
Copy link
Contributor Author

Hmm, that patch worked on my local (which did not include automated tests — guess I need to at least get testing up to be able to run those before further PRs). But @eileenmcnaughton's suggestion should take care of it. PR updated.

@eileenmcnaughton
Copy link
Contributor

Adding has-test as I have written a test that captures this bug & demonstrates the fix works - I will push up as a follow up PR - merging

@eileenmcnaughton eileenmcnaughton merged commit b142252 into civicrm:master May 20, 2021
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request May 20, 2021
seamuslee001 added a commit that referenced this pull request May 20, 2021
@bugfolder bugfolder deleted the mandatory_keys_check branch May 20, 2021 18:56
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