Api4 - Prevent developer error mixing up addValue with addWhere

[colemanw]

Overview

This adds a sanity check in the api to prevent the common mistake of accidentally using addValue as if it takes 3 arguments like addWhere.

Technical Details

This is an easy mistake to make because the function is similar to addWhere:

Wrong:

Address::create()
  ->addValue('city', '=', 'Paris')

Right:

Address::create()
  ->addValue('city', 'Paris')

Now the API will throw an exception telling you what you did wrong.

Comments

Instead of pasting this check in 5 places I used a trait.

[civibot]

(Standard links)

• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[mattwire]

Don't think I've ever got it wrong that way around but I do keep forgetting to put the operator in addWhere()!

[colemanw]

@mattwire doing that also throws an exception :)

[totten]

Ah, nice. I don't know if reflective performance still matters these days, but we don't have to worry - since it only needs to check in a small edge-case.

[elisseck]

• General standards

  • r-explain: pass
  • r-user: pass
  • r-doc: pass
  • r-run: undecided
    Ran via cv php:scr under php 8.0:

  $results = \Civi\Api4\Contact::create(FALSE)
  ->addValue('contact_type', 'Individual')
  ->addValue('first_name', '=', 'Test')
  ->addValue('last_name', 'addValue')
  ->execute();
  

  Previous Behavior: Runs without issue, results in an incorrect contact with first name '='.
  New Behavior: Receive API error "APIv4 function addValue incorrectly called with 3 arguments."

Only undecided here to note that it appears that now we will receive API errors where we didn't before (even if the behavior was incorrect). That may be fine however. These should probably be in try/catch anyways.

On mattermost (https://chat.civicrm.org/civicrm/pl/5apjsytjw7gwmrsy7dq1386nfe) it seems like the thought was this would throw an error, however, if the type was incorrect. I wasn't able to make this happen with a contribution create date either ("now" was filled instead for "date_received" in this case).

• Developer standards
  • r-tech: pass (love the use of trait)
  • r-code: pass
  • r-maint: pass
  • r-test: pass (coleman noted on mattermost there is great test coverage here already)

[colemanw]

Thanks for the review - I agree that try/catch should be used. IMO any developers who have done this incorrectly and now get an error will benefit in the long run, as will any future developers who make this mistake!