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

api4 explorer: make boolean params work in cv (short syntax) #25589

Merged

Conversation

highfalutin
Copy link
Contributor

Overview

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

Before

In API v4 explorer, boolean params are rendered in CV (short) syntax as true or false. The generated code does not work in cv.

After

Params are rendered as 1 or 0. Generated code works in cv.

@civibot
Copy link

civibot bot commented Feb 15, 2023

(Standard links)

@civibot civibot bot added the master label Feb 15, 2023
@highfalutin
Copy link
Contributor Author

Well actually in some cases the cv command still doesn't work... cv interprets all param values given in key=value syntax as strings. Even if we pass, for example, useTrash=0 instead of useTrash=false, the param value will still be string "0" and so type-checking will fail and throw an error.

In at least one case (\Civi\Api4\Generic\AbstractAction::setCheckPermissions()), the string is explicitly recast to a boolean, and this works correctly for checkPermissions=0. But for checkPermissions=false, the string "false" is recast to boolean true, and no error is thrown (except maybe "Authorization Failed").

@totten
Copy link
Member

totten commented Feb 16, 2023

@highfalutin I think your original intuition is pretty good -- passing 0/1 on CLI generally works better than passing "false"/"true". This is historically true in similar interfaces (like cv api3 and drush cvapi); it's true in related ecosystems (like C and MySQL); and it checks out for several things in cv api4:

  • checkPermission=0: Numeric-bool works. String-bool doesn't.
  • +v do_not_sms=1: Numeric-bool works correctly. String-bool doesn't.
  • +w do_not_phone=0: Numeric-bool works correctly. String-bool doesn't.

That is curious that useTrash differs. I think the difference between useTrash and checkPermissions is this:

(To confirm this is the difference: you can add a real setter method for useTrash, and it cv api4 Foo.delete useTrash=0 will work.)


IMHO, this patch is better than what it was doing before. 👍

Aside: There is a related issue in how the explorer renders booleans in the "Where" (+w) and "Value" (+v) clauses. But that doesn't have to stop this.

@eileenmcnaughton eileenmcnaughton added the merge ready PR will be merged after a few days if there are no objections label Feb 16, 2023
@eileenmcnaughton
Copy link
Contributor

I added merge-ready based on @totten's "IMHO, this patch is better than what it was doing before. "

I think some related issues came up - but they don't need to be in scope for this PR so I'll just let @highfalutin have a chance to read & respond

@highfalutin
Copy link
Contributor Author

@totten thanks for the comments, and thanks for #25595 which goes a long way to addressing the issues I mentioned in #25589 (comment) above.

In addition to the +where problem you mentioned, I think it's also not ideal for strings to get turned into booleans silently in some cases (I'm looking at you, setCheckPermissions()), but none of that should prevent this PR from getting merged.

@totten totten merged commit 5046f62 into civicrm:master Feb 17, 2023
@highfalutin highfalutin deleted the api4-explorer-cv-syntax-short-boolean branch February 17, 2023 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants