civicrm / org.civicrm.api4 Goto Github PK

I've seen some comments in the tests like this:

// fixme - what am I doing wrong to need this line?
require_once 'UnitTestCase.php';

I guess it's because the tests aren't in the "Civi" directory. After messing around a bit I have an idea that the Civi stuff is autoloaded properly because of the Civix namespace in info.xml.

I think having tests as a top level folder is the right approach, and would even like to put them in a different namespace root. To do this it seems like we should just be able to add to the classloader attribute in info.xml.

But when adding the classloader to the info.xml it seems to add it to the autoloader's prefixesPsr0 instead of PSR-4 as the XML indicates.

The only composer PSR-4 prefix I see in the instantiated autoloader is:

Civi\\Cxn\\Rpc\\ => /path/to/civicrm/vendor/civicrm/civicrm-cxn-rpc/src

In addition it doesn't seem to use the extension directory for the path. For example when I add new classloader mapping it just uses the CiviCRM root at the relative path root, for example adding:

<psr4 prefix="Civi\" path="tests" />

To the extension info.xml will produce the following PSR-0 mapping in the autoloader:

"Civi" => "/path/to/civicrm/tests/phpunit"

But of course I don't plan to add my code to the CiviCRM root.

I don't think we can fix this without a core change (unless I've understood the root of the problem).

Edit:
I think maybe I had the wrong idea and custom namespaces from info.xml are not loaded in the tests, but it would be nice if they were.

Both the current Delete and Update action classes seem to operate on a Get list.

Both contain this boilerplate:

parent::_run($result);       
// Then act on the result    
foreach ($result as $item) { 
  // update it               
}                            
return $result;              

Should both Delete and Update be subclasses of a new abstract "bulk amender" class?

This would allow some batching, queuing, job control magic to be centralised over not just these 2 core actions but also other "update one or more" type actions?

I have been discussing the API4 with @colemanw lately (thanks for all the pointers @colemanw!!)

There is one thing that is troubling me a bit though and this is the trade-off between getting progress with an early prototype MVP vs the notion of "getting it right".

I have heaps of sympathy for the idea of "keep it simple and just get it working"... except this is Version 4 we are talking about.

It feels to me like Version 3 is the MVP and we need to pause and address a few issues (notably around scalability and performance).

But I'm just trying to get a sense of how urgent the V4 MVP is. Is it urgent and I should focus effort on V5 (omg)... or do we have a bit of time to try and work up some more audacious goals for V4??

So I think this means caching, asynchronous, supporting parallelisation with an eye to supporting tools like rabitmq and redis to break beyond the constraints of a LAMP stack)

But I really have no true detailed feel of what this looks like... but the need to add this principle in is one of the biggest concerns I have with rushing to a MVP (#13) because I am worried that it may impose limitations or need patterns that require breaking earlier patterns

(edit: the vision is to support DBs with a million+ contacts and busy nation-wide teams - these organisations are currently turning to commercial alternatives and taking their resources to fund dev work on civi with them!)

so this had me stumped:

.PHP Fatal error:  Uncaught PEAR_Exception: DB Error: unknown error in unknown on line unknown
 DB_Error: DB Error: unknown error in unknown on line unknown
#0 [internal function]: CRM_Core_Error::exceptionHandler(Object(DB_Error))
#1 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/packages/PEAR.php(921): call_user_func(Array, Object(DB_Error))
#2 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/packages/DB.php(985): PEAR_Error->__construct('DB Error: unkno...', -1, 16, Array, 'ROLLBACK TO SAV...')
#3 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/packages/PEAR.php(575): DB_Error->__construct(-1, 16, Array, 'ROLLBACK TO SAV...')
#4 [internal function]: PEAR->_raiseError(Object(DB_mysqli), NULL, -1, NULL, NULL, 'ROLLBACK TO SAV...', 'DB_Error', true)
#5 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/packages/PEAR.php(224): call_user_func_array(Array, Array)
#6 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/packages/DB/common.php(1905): PEAR->__call('raiseError', Array)
#7 /opt/buildkit/build/dmast in /opt/buildkit/build/dmaster/sites/all/modules/civicrm/CRM/Core/Error.php on line 938

the error log contained:

Mar 01 07:32:24  [info] $backTrace = #0 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/CRM/Core/Error.php(937): CRM_Core_Error::backtrace("backTrace", TRUE)
#1 [internal function](): CRM_Core_Error::exceptionHandler(Object(DB_Error))
#2 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/packages/PEAR.php(921): call_user_func((Array:2), Object(DB_Error))
#3 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/packages/DB.php(985): PEAR_Error->__construct("DB Error: unknown error", -1, 16, (Array:2), "ROLLBACK TO SAVEPOINT civi_2 [nativecode=1305 ** SAVEPOINT civi_2 does not ex...")
#4 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/packages/PEAR.php(575): DB_Error->__construct(-1, 16, (Array:2), "ROLLBACK TO SAVEPOINT civi_2 [nativecode=1305 ** SAVEPOINT civi_2 does not ex...")
#5 [internal function](): PEAR->_raiseError(Object(DB_mysqli), NULL, -1, NULL, NULL, "ROLLBACK TO SAVEPOINT civi_2 [nativecode=1305 ** SAVEPOINT civi_2 does not ex...", "DB_Error", TRUE)
#6 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/packages/PEAR.php(224): call_user_func_array((Array:2), (Array:8))
#7 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/packages/DB/common.php(1905): PEAR->__call("raiseError", (Array:7))
#8 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/packages/DB/common.php(1905): PEAR->raiseError(NULL, -1, NULL, NULL, "ROLLBACK TO SAVEPOINT civi_2 [nativecode=1305 ** SAVEPOINT civi_2 does not ex...", "DB_Error", TRUE)
#9 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/packages/DB/mysqli.php(933): DB_common->raiseError(-1, NULL, NULL, NULL, "1305 ** SAVEPOINT civi_2 does not exist")
#10 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/packages/DB/mysqli.php(403): DB_mysqli->mysqliRaiseError()
#11 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/packages/DB/common.php(1216): DB_mysqli->simpleQuery("ROLLBACK TO SAVEPOINT civi_2")
#12 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/packages/DB/DataObject.php(2438): DB_common->query("ROLLBACK TO SAVEPOINT civi_2")
#13 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/packages/DB/DataObject.php(1627): DB_DataObject->_query("ROLLBACK TO SAVEPOINT civi_2")
#14 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/CRM/Core/DAO.php(354): DB_DataObject->query("ROLLBACK TO SAVEPOINT civi_2")
#15 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/Civi/Core/Transaction/Frame.php(163): CRM_Core_DAO->query("ROLLBACK TO SAVEPOINT civi_2")
#16 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/Civi/Core/Transaction/Manager.php(117): Civi\Core\Transaction\Frame->finish()
#17 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/CRM/Core/Transaction.php(142): Civi\Core\Transaction\Manager->dec()
#18 /opt/buildkit/build/dmaster/sites/all/modules/civicrm/Civi/Test/CiviTestListener.php(72): CRM_Core_Transaction->commit()
#19 phar:///opt/buildkit/bin/phpunit4/phpunit/Framework/TestResult.php(340): Civi\Test\CiviTestListener->endTest(Object(Civi\API\V4\ParticipantTest), 0.13832211494446)
#20 phar:///opt/buildkit/bin/phpunit4/phpunit/Framework/TestResult.php(733): PHPUnit_Framework_TestResult->endTest(Object(Civi\API\V4\ParticipantTest), 0.13832211494446)
#21 phar:///opt/buildkit/bin/phpunit4/phpunit/Framework/TestCase.php(724): PHPUnit_Framework_TestResult->run(Object(Civi\API\V4\ParticipantTest))
#22 phar:///opt/buildkit/bin/phpunit4/phpunit/Framework/TestSuite.php(747): PHPUnit_Framework_TestCase->run(Object(PHPUnit_Framework_TestResult))
#23 phar:///opt/buildkit/bin/phpunit4/phpunit/Framework/TestSuite.php(747): PHPUnit_Framework_TestSuite->run(Object(PHPUnit_Framework_TestResult))
#24 phar:///opt/buildkit/bin/phpunit4/phpunit/TextUI/TestRunner.php(440): PHPUnit_Framework_TestSuite->run(Object(PHPUnit_Framework_TestResult))
#25 phar:///opt/buildkit/bin/phpunit4/phpunit/TextUI/Command.php(149): PHPUnit_TextUI_TestRunner->doRun(Object(PHPUnit_Framework_TestSuite), (Array:5))
#26 phar:///opt/buildkit/bin/phpunit4/phpunit/TextUI/Command.php(100): PHPUnit_TextUI_Command->run((Array:1), TRUE)
#27 /opt/buildkit/bin/phpunit4(545): PHPUnit_TextUI_Command::main()
#28 {main}

after a bit of snooping I found i could fix this by commenting out a couple of lines in civicrm/Civi/Test/CiviTestListener.php:

 70   public function endTest(\PHPUnit_Framework_Test $test, $time) {
 71     if ($test instanceof TransactionalInterface) {
 72 //      $this->tx->rollback()->commit();
 73 //      $this->tx = NULL;
 74     }

not sure about next steps...

seems worth adding this motherhood statement?

worth unpacking a bit more what secure means and how to make clear to both internal external coders their security privilege context?

Api3 transforms the string 'null' to the value NULL. This will be quite problematic for Mr. and Mrs. Null when you try to enter their names in a form.

The api3 also has a problem where it ignores the value NULL which makes it impossible to unset an existing value using the api update action (without using the above hack).

A/C:

• Api4 should keep the string 'null' as a string and not transform it.
• Api4 should allow a value to be set as NULL from PHP or javascript.
• Test to demonstrate the above.

Further to #13 #14 #41, a short braindump of things which I'd like to see addressed more clearly in api4. I'm considering these implications for both internal and external API consumers, and for CiviCRM going forward.

• #41 - The ability to construct arbitrary / client-driven SQL means that exposed API has to work a lot harder to protect the application performance and data from attackers and bad programming. It also introduces complexity in other aspects of the API, such as caching.
• #18, #20 - Paging is an essential feature of API queries, but I saw discussion that to expose paging we'd need to expose internal tables which left me uncomfortable. Exposing a new set of (internal) data provides new ways for things to break that an API is supposed to protect against.
• The idea (from chat.civicrm.org) that API queries could pass in cache details feels pretty wrong to me. Maybe I misunderstood the suggestion, but I'd think the application not the client should determine what caching is OK, and offering clients the ability to request outdated cache results has no advantage. I'd feel more OK with clients being able to "force-refresh" a resultset perhaps than for them to pass in a "cache_not_before" param?

Opening this ticket to document a few items that I've seen discussed and where I think it would be good to set a direction on how to approach.

Paging is useful for rapid UI pages, and also to facilitate batch operation.

So it seems useful to think about 3 levels of entity lists:

• simple bag of IDs [57, 108, 130]
• sortable (therefore pageable) list - which has the ID and a single field that enables sorting
• decorated lists with extra fields from same enitity or foreign tables

Architecture needs to start with simple lists to allow Boolean "set" operations (union, intersection, subraction)/(OR/AND/NOT) and then once Boolean phase is over the list maybe made sortable.

I'm still not sure what the above means practically but wanted to start organising thoughts...

then finally the list is decorated on a "page by page" basis

both of the first 2 levels will benefit form caching

looking at these two classes:

• https://github.com/civicrm/civicrm-core/blob/master/tests/phpunit/api/v3/ParticipantTest.php
• https://github.com/civicrm/civicrm-core/blob/master/tests/phpunit/CiviTest/CiviUnitTestCase.php
• https://github.com/civicrm/civicrm-core/blob/master/tests/README.md

there is a lot of sugar in the latter in CiviUnitTestCase

is there a reason not to extend it here?

• https://github.com/civicrm/api4/blob/master/tests/phpunit/Civi/API/V4/ParticipantTest.php

Api4 should treat each multi-record custom data set as its own CRUD entity.

Proposed syntax (object instantiation is a slight deviation from the normal pattern to accommodate a dynamic entity class):

Civi\Api4\CustomValue::get('MyRecords')
  ->addWhere('entity_id', '=', 123)
  ->execute();

just creating a case to dump ideas around how API4 should support caching (or not)

it keeps coming up in unrelated issues but I think caching needs a base issue to nut it out.

it is related to #20

It would be fairly easy to tweak the syntax so that you can add the same field twice to a query. Use case:

  \Civi\Api4\Contact::get()
    ->addWhere('last_name', 'LIKE', 'smi%')
    ->addWhere('last_name', '!=', 'smith')
    ->execute();

advice from Eileen is to check hook tests in the api v3 tests - e.g api_v3_ACLPermissionTest

I can understand the need for 2 Get classes (I guess) but is 3 really necessary?

     ./Civi/API/V4/Action/Get.php:40:class Get extends API\V4\Action             
     ./Civi/API/V4/Entity/Entity/Get.php:34:class Get extends Action             
     ./Civi/API/V4/Entity/Participant/Get.php:34:class Get extends \Civi\API\V4\Action\Get                                                                       

another item from the readme that is a bit opaque to me...

I think I understand but I can smell 🐉

coming from discussion #29

API 4.alpha.0.0.0.1 is likely to be clean & fast contribution.get action...

need to specify the specific requirements (by contact? by campaign? what is good about v3 and needs to come over? etc

any other general requirements be good to know too

For CiviCRM using its own API internally as a query builder, I think #10, #11, #31 etc make a fair bit of sense.

When we expose the API to external clients, I feel uncomfortable about our API being an SQL builder, because we're exposing a lot more surface to attackers to pry on when we provide direct SQLisms such as LIKE, OR, AND, NOT. (Compared with API queries being limited to simple entity filters by matching parameter.)

I understand that historically CiviCRM has exposed a lot of SQLism, that the application is deeply SQL-ish, and that not offering this would be a big challenge. But I want to check in and see whether we can approach this in a way which improves our security.

Here I see CiviCRM's API trying to do things other APIs typically don't, which makes me wonder if a unique approach is a good idea. Are there good examples of other web APIs which expose SQL params so directly?

For me, this raises the question of whether building the API for dual-usage (internal and external) places conflicting demands on the resulting tool.

just tidying up the test file and found this comment.

is there a strong view? shall we lock it in? then just needs documenting in the readme?

    $call = Participant::get()                                                  
      ->setLimit(2);                                                            
    $result = $call->execute();                                                 
    $this->assertEquals(2, count($result),                                      
      "did not get back two records as expected");                              
    // Here's a convenient way to get the first result - maybe a replacement for getsingle                                                                      
    // Rationale for ditching getsingle - it's an output format & not a real action                                                                             
    // and output transformations would be better handled by the $result object.
    $firstResult = $result->first();                                            
    $this->assertEquals(1, $firstResult['id']);                                 

@eileenmcnaughton pointed me at the conformance test as a good pattern to follow for API4.

I'm seeing there is a bunch of entity-level and action-level metadata in there, in various flavours of:

• implemented,
• "to be implemented",
• "not to be implemented"
• is updateable (field level?)
• is deleteable

I API4 should publish this information via getActions() and getFields() in order to make it discoverable?? and should there be a centralised 'source of truth'??

following on from #14, complementing #29 (which deals with getting some simple plumbing quickly) and concerns about scope and achievability and actually doing something useful...

what are the highest priority "complex, business-process oriented" API actions we need to address in V4?

from the wishlist in the Readme - I think I understand this one

I'm doing this partly because I need to pull it apart to understand and because it maybe useful to others.

Am aware that other's extending the project in the future may have limited php exposure so am deliberately trying to aim at a moderately introductory level.

WIP is over hear and feedback welcome: https://github.com/ErichBSchulz/api4

(I'll aim to get a PR in next week or two)

arising from concerns @ #41 - just need to ensure we are creating security vulnerabilities by allowing standing clauses to be NOTed

In api v3, you pass params like 'custom_123' => 'foo'. This is not portable because the id of custom fields can change when migrating a site e.g. from dev to staging.

Suggested syntax in api4 (using the "group" + "name" of a custom field):

->addWhere('a_few_of_myfavorite_things.favorite_color', 'IN', ['blue', 'yellow'])

https://wiki.civicrm.org/confluence/display/CRM/Symbolic+Names+for+Custom+Data+and+API

This could be yet another Design Principle (arising from @eileenmcnaughton's comment on #14)

ie:

• clear distinction between what is a pure CRUD "plumbing" operation and what is a complex "porcelain operation"
• ensuring side-effects are clearly documented, universally appropriate or explicitly configurable

it is half done by looks of it

Hi,
The example code is

\Civi\Api4\Contact::get()
  ->setSelect(['id', 'sort_name'])
  ->addWhere('contact_type', '=', 'Individual')
  ->addOrderBy('sort_name', 'DESC')
  ->setCheckPermissions(TRUE)
  ->execute();

what are the benefits of the "set" and "add" orefix? Wouldn't removing them make it simplier?
it might be a non native issue, but I would as happily use setWhere or addSelect. ie. that prefix feels that it can be either, and I wouldn't know the rule of deciding which one I'm supposed to put.

either clarifying, or simply removing them:

\Civi\Api4\Contact::get()
  ->Select(['id', 'sort_name'])
  ->Where('contact_type', '=', 'Individual')
  ->OrderBy('sort_name', 'DESC')
  ->CheckPermissions(TRUE)
  ->execute();

or (more active record style)

\Civi\Api4\Contact
->Select(['id', 'sort_name'])
->Where('contact_type', '=', 'Individual')
->OrderBy('sort_name', 'DESC')
->CheckPermissions(TRUE)
->get();

from Eileen: "check api_v3_SyntaxConformanceTest - it makes heavy use of data providers to iterate through all api actions for certain things"

Overview

To avoid breaking anything we should run tests automatically on each pull request and show the results in the PR comments.

The api3 has a convention of using magic-named callback functions e.g. _civicrm_api3_activity_create_spec() to modify the list of dao fields for the api consumer.

We need some similar functionality in api4, but something using events would be better. If there isn't already a suitable event being fired, let's add one.

Acceptance criteria:

• An event which allows each individual entity api to alter field metadata.
• Fields can be altered based on the action being performed (e.g. Contact::create can have different fields from Contact::get.
• Setting the flag api.required on a field for a particular action will result in the api rejecting a request in which that field is empty.
• Setting the flag api.default on a field for a particular action will result in that param automatically being supplied by the api wrapper (if not specified in the api request).
• Unit tests to demonstrate the above.

(arising from @eileenmcnaughton's comment on #14, and supporting #27 and #28)...

how do we model or specify the metadata around the actions? is it useful/possible to classify porcelain/plumbing in some kind of useful way? ie

• to drive the API itself
• to drive testing
• to signpost the danger spots ("yes you can delete the contribution but only if you indicate you are doing this as some kind of more complex porcelain operation")

for example what are:

• the entity records where a create action is a valid independent transaction?
• the entity records where a delete action is a valid independent transaction?
• the entity fields where an update action is a valid independent transaction?

IMHO, in order to optimise the API and CiviCRM more broadly we need solid data on what tasks are taking up time

this maybe bigger than API4 but seems to me that capturing and logging performance data is a criticial requirement

so yet another one I'd like a +1 or -1 on...

I sketched a rough plan here of what this could look like and why its worth doing.

We need a way for each api action to tweak its params without necessitating overriding the parent action.

A/C:

• Event that each api can respond to to format their params on a per-action basis.
• Refactoring code from https://github.com/civicrm/api4/pull/60/files/a5340a524ef2365b84399a0951adc2c6ad40aeab#diff-99ccf38a589eb38555647d760fa495ba

this is also half done...

and it needs to extend Get class not the Action class

http://jsonapi.org/

we currently have an ParticipantTest::testGet()

which implies we need a testCreate(), testUpdate() and a testDelete()

the problem with this pattern is it will be a bit slow spread over mulitple enitities (on my machine the test is taking 1.4seconds already! - one action one entity)

so it seems it would be good to have a testCrud() pattern:

1. truncate table
2. assert record count = 0
3. create n (1...5 records? depending on mood etc?)
4. assert count = n
5. get interesting records
6. update records
7. reget interesting records
8. delete some records
9. verify expected records gone
10. delete all records
11. assert record count = 0

how does that look?

Static calls are fine for something that has no internal state, the CRM_Utils_Array class, for example, is very helpful. However by using a static based approach for the API we make it hard to test new classes, and harder to keep track of dependencies.

For example take a class that tries to find duplicate contacts. If I want to test the logic for finding duplicates I need to create them in the database if it fetches contacts via static calls. This adds a lot of setup time, and could result in errors if there is conflicting data in the database. If I inject a ContactApi instance as a constructor argument I can use a mock in tests that will pass me what I expect in the tests without touching the database.

Furthermore, if I decide in DupeFinder::findDupes() to add a flag to do something related to another entity, for example returning all contact emails with the$withEmails flag then I'd need to add the EmailApi via the constructor. Some people might recoil and think this is tiresome, but by forcing the developer to realize how many related classes their class depends on I think we end up encouraging smaller classes, focused on a single responsibility.

Faster tests will make people more likely to write lots of them, covering more edge cases. Database set-up and tear down time for a single test is not huge right now, but as we have more and it becomes a major task to run all unit tests, as it is now in CiviCRM core.

It is possible to write wrapper classes that hides the static calls and inject that as a constructor argument to my class, but eventually we would have a lot of wrapper classes just to avoid the static calls.

My suggestion is to make the API use the regular OO structure of inheritance. Instead of having a base Get class, have an abstract BaseAPI class with the basic methods. Extend from this class to add new entity APIs. This API class could then be injected in the class, which would both help with testing and force you to recognize your class dependencies. If you had a class where you were injecting 20 API classes you might consider splitting it into smaller classes (which I think is good practice), but with 20 static API calls you probably won't notice this.

Here is some psuedocode to illustrate my suggested approach:

abstract class BaseAPI {
  /**
   * ParameterBag is just a suggestion, since it
   * would be nice to allow an OO approach to setting parameter values. It
   * would probably be similar to what we currently have for Action and
   * allow array access. It might be an option to allow an array as well.
   *
   * @param ParameterBag $params
   *
   * @return Result
   */
  public function get(ParameterBag $params) {
    // from here run the request through kernel and return result
  }

  // all the other base actions
}

class ContactAPI extends BaseAPI {
  /**
   * @param ParameterBag $params
   *
   * @return Result
   */
  public function get(ParameterBag $params) {
    // just an example if we want to enforce some behaviour unique to this entity
    $params->setOption('limit', 20);

    return parent::get($params);
  }
}

/**
 * Sample class showing usage of suggested style
 */
class DupeFinder {
  /**
   * @var ContactAPI
   */
  protected $contactApi;

  /**
   * @param ContactAPI $contactApi
   */
  public function __construct(ContactAPI $contactApi) {
    $this->contactApi = $contactApi;
  }

  /**
   * @return array
   */
  public function findDupes() {
    $dupes = array();
    $allContacts = $this->contactApi->get();

    foreach ($allContacts as $contact) {
      $params = new ParameterBag();
      $params->setValue('display_name', $contact['display_name']);
      $dupeResults = $this->contactApi->get($params);

      if ($dupeResults->getCount() > 0) {
        $dupes += $dupeResults;
      }
    }

    return $dupes;
  }
}

Testing the DupeFinder would be a lot easier then, you could just inject a mock and test a lot of different scenarios without having to setup the database.

Internally it doesn't seem that V4 or some of the V3 SQL logic keeps a more detailed record of security than "need to check? yes/no"

I think this may be as simple as expanding the boolean flag to have a range of values

but it could be hair and I sense shadows and pitfalls...

"permitted to edit", "permitted to add tag X or contact Y"

This seems like a start - but there has to more: https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Permission.php#L49-L54

this scares me :-)

Civi\API\V4\Action::_run() is empty...

should it throw an exectption if this is by design?

or can it call something in the BAO??

Sorry this is another hairy subjective one. But posting this get some guidance on approach and philosophy.

The current code base is essentially a CRUD wrapper around the BAOs. Is this what we see as most critical in V4? if not what is the shape of the business operations that the API needs to support and what is the pattern?

background

"Avoid designing a REST interface that mirrors or depends on the internal structure of the data that it exposes. REST is about more than implementing simple CRUD (Create, Retrieve, Update, Delete) operations over separate tables in a relational database. The purpose of REST is to map business entities and the operations that an application can perform on these entities to the physical implementation of these entities, but a client should not be exposed to these physical details." "Avoid introducing dependencies between the web API to the structure, type, or location of the underlying data sources. For example, if your data is located in a relational database, the web API does not need to expose each table as a collection of resources. Think of the web API as an abstraction of the database, and if necessary introduce a mapping layer between the database and the web API. In this way, if the design or implementation of the database changes (for example, you move from a relational database containing a collection of normalized tables to a denormalized NoSQL storage system such as a document database) client applications are insulated from these changes." ref

(edit) some concrete examples:

• "update Participation" vs "confirm contact attended event as anticipated"
• "delete event" vs "cancel event and credit attendees accounts with fee paid"
• "update contact address" vs "record fact that contact has moved"
• "create contribution" vs "record donation"

(/edit)

What are the advantages of keeping the traditional syntax?

I think this syntax goes against 2 of the Design Principle on the README file:

• Clean - leave all the legacy cruft in v3 and start with a clean slate
• OOP - use classes in the \Civi namespace - minimize boilerplate via class inheritance/traits

I doesn't look clean to me to have 2 syntaxes for the same thing. Regarding the OOP principle, I understand it's more about the internal classes used by the API than its interface, but I think that, for consistency, it would be better to have a single OOP syntax/interface for the API

They syntax could easily get messy. Need to think of a clean-ish way to support groupings of AND and OR.

This is just a response to comments from @colemanw in Mattermost, but my response was too long and thought it would be more readable here. Also it might be good to post here since anyone can comment.

• It's broken Entity::get() for fetching the list of entities.

There was a test for that, and I should have realized I'd broken it. I assumed it hadn't worked before, because when I started work on this many of the existing tests were failing. Seems like you've fixed it now because this test is working, thanks. Hopefully we can get to to the stage where all tests are working soon, and keep them working. ParticipantTest and ConformanceTest are being skipped now as they were broken.

• This raises a question of whether we ought to have the BaseEntity class in the same directory as actual entities. For the sake of the way Entity::get() scans the directories, I'd vote "no".

I wouldn't mind moving it somewhere, but my first concern would be where it most naturally belongs. If it's just to satisfy a method it seems like just changing the method might be a better option than putting the class in a directory where nobody would expect it to be. Seems like you've fixed the directory scanning to work around this. Also see my response below about AbstractAction.

• I'm slightly concerned that your psr4 trick might be too clever. We want it to be as straightforward as possible for other extensions to add api4 entities. A simpler albeit slightly longer directory structure may be better. What is the problem you're solving by placing the Entity and Action directories in the Civi root directory?

PSR-4 is cleaner as it avoids these useless directories that only contain a single subfolder, which is why I used it here. Also, PSR-0 is deprecated.

• How would other extensions add entities and actions without needing to use that psr4 trick?

The Civi\ PSR-0 namespace is used in Core now, because it's configured like this in composer.json. From 4.7 we allow extensions to specify their own namespaces in info.xml. If I'm an extension author I can use PSR-4 to mimic core directory structure, or I can put my new entities wherever I want. As a test I created a sample extension and installed it in a unit test. The sample extension added two namespaces to info.xml:

  <classloader>
    <psr4 prefix="Civi\" path="Civi" />
    <psr4 prefix="Civi\API\V4\Entity\" path="ExtensionEntities" />
  </classloader>

The first one to demonstrate how the extension author can stick to the same directory structure as core if they want to, the second to show the flexibility of PSR-4. This is the directory structure in my extension:

├── Civi
│   └── API
│       └── V4
│           ├── Action
│           │   └── Country
│           │       └── Foo.php
│           └── Entity
│               └── Country.php
├── ExtensionEntities
│   └── County.php

With this in place the following code worked:

    $result = civicrm_api4('Country', 'foo', ['checkPermissions' => 0]);
    $result2 = civicrm_api4('County', 'get', ['checkPermissions' => 0]);

Neither of these entities are defined in api4, only in the extension. The foo action was correctly called and returned the expected content.

So, that's how extensions can add entities and actions. I'm not sure if you meant you wanted to support the "CRM" Pear autoloading style, which I'd avoid if possible.

Another alternative way to register entities / actions, could be with tagged services, which would allow extensions to put these classes anywhere and we wouldn't have to worry about only searching a certain namespace. One question that I haven't really asked yet is how we'd handle clashes, for example if two extensions try to customize Contact::get().

• This could be mitigated if there were a way to make Entity::get() scan according to the psr4 rules. But I don't know of a way to do that off the top of my head, and I'd rather follow the Civi convention of always matching directory structures to class names rather than have to work around all the problems departing from that standard may cause.

Until recently it seems the Civi convention was PEAR autoloading, and seems it still is in many places. I wasn't aware the PSR-0 was the offical autoloading style of Civi, I saw PSR-4 support in info.xml and decided in the restructuring to use it.

• having AbstractAction in the same directory as the real actions also messes up the scanning, as well as being confusing (Civi convention is to always put parent classes one directory up)

I wasn't aware this was the Civi convention. I'm not sure if it's confusing for everyone, if I was looking for the AbstractAction class and there's a root directory called "Action" I probably wouldn't take too long to find it, and I'm not the sharpest. Open to changing it if it is convention in core.

• On second thought I'm not very fond of the new namespace. \Civi\Api4\Contact::get() was easy to type & remember. \Civi\API\V4\Entity\Contact::get() is less elegant as far as the api consumer is concerned.

You're right. I was thinking about this after it was merged. We should follow the Vendor\Project, so Civi\Api4 sounds better.

What to do next:

• Get all the tests working so we don't break things with future changes.
• I agree with the Civi\Api4 namespace so I think we should change it.
• If the convention is to put abstract classes one level up then we can move AbstractAction and BaseEntity to root. I would avoid renaming them to Action and Entity as this would lead to an overlap of class name and namespace root that I mentioned before.
• I would prefer to see some more discussion before discarding PSR-4, as it means we're committing to a standard that's been deprecated for almost 3 years.

I don't know what this means but it is in the wishlist (I'm going to delete the wishlist in readme shortly in favour of the issue queue if that is ok?).

I think this one is a no-brainer?

(although it may redefine and complicate what constitutes an MVP)

I encountered this recommendations for what at least one random internet person considered to be examples of excellent and detailed documentation: Twilio, Django, and MailChimp (for reference)

Example queries that we want to be able to write:

1. Fetch all activities for housing support cases
2. Fetch all activities with a blue tag; and return all tags on the activities
3. Fetch contacts named 'Bob' and all of their blue activities
4. Get all contacts in a zipcode and return their Home or Work email addresses
5. Fetch all activities where Bob is the assignee or source
6. Get all contacts which (a) have an address in zipcode 94117 or 94118 or in city "San Francisco","LA" and (b) are not deceased and (c) have a custom-field "most_important_issue=Environment".
7. Get participants who attended CiviCon 2012 but not CiviCon 2013. Return their name and email.

We can relate/learn from some examples:

• APIv3 chaining can do some of this, but it's decidedly non-performant, and some of the joining semantics are invisible/magic.
• GraphQL can do some of this, but it doesn't get into filtering notations.

I downloaded the api & as step number 1 ran the tests. They do not currently pass - due to a change @colemanw added recently to set the default for check_permissions to be TRUE.

Both getActions and the get call are failing

I would suggest we set the tests to use setCheckPermssions = FALSE so they pass again since we should always have the tests in a passing state & this is a new barrier

I'm not sure if this is expected or not:

root@209e1d6cabeb:/opt/buildkit/build/dmaster/sites/default/files/civicrm/ext/api4# phpunit4
PHPUnit 4.8.21 by Sebastian Bergmann and contributors.

Installing dmastertes_w0qpz schema
EE..

Time: 1.61 minutes, Memory: 33.50Mb

There were 2 errors:

1) Civi\API\V4\ParticipantTest::testGetActions
Civi\API\Exception\NotImplementedException: API (Participant, getActions) does not exist (join the API team and implement it!)

/opt/buildkit/build/dmaster/sites/all/modules/civicrm/Civi/API/Kernel.php:223
/opt/buildkit/build/dmaster/sites/all/modules/civicrm/Civi/API/Kernel.php:166
/opt/buildkit/build/dmaster/sites/default/files/civicrm/ext/api4/Civi/API/V4/Action.php:161
/opt/buildkit/build/dmaster/sites/default/files/civicrm/ext/api4/tests/phpunit/Civi/API/V4/ParticipantTest.php:18
/opt/buildkit/bin/phpunit4:545

2) Civi\API\V4\ParticipantTest::testGet
Civi\API\Exception\NotImplementedException: API (Participant, get) does not exist (join the API team and implement it!)

/opt/buildkit/build/dmaster/sites/all/modules/civicrm/Civi/API/Kernel.php:223
/opt/buildkit/build/dmaster/sites/all/modules/civicrm/Civi/API/Kernel.php:166
/opt/buildkit/build/dmaster/sites/default/files/civicrm/ext/api4/Civi/API/V4/Action.php:161
/opt/buildkit/build/dmaster/sites/default/files/civicrm/ext/api4/tests/phpunit/Civi/API/V4/ParticipantTest.php:38
/opt/buildkit/bin/phpunit4:545

FAILURES!
Tests: 4, Assertions: 5, Errors: 2.

we're moving the dev docs... would be good to move this too:

https://wiki.civicrm.org/confluence/display/CRM/API+v4+Spec

Most can be just dumped into the README

and the rest made into PRs and issues?