systopia / de.systopia.eck Goto Github PK

Per civicrm/civicrm-core#24164 the list of entities included in the Recent items list in the navigation menu can be customized. Let's add ECK entities to that list. This should be configurable per entity type.

Placeholder for an issue to be fixed in CiviCRM Core, see /dev/core#5103.

This will be possible now with the new event added in 5.50.

See: civicrm/civicrm-core#23303

Example use: https://lab.civicrm.org/extensions/form-processor/-/merge_requests/36

 ------ --------------------------------------------------------------------------- 
  Line   CRM/Eck/Page/Entity/View.php                                               
 ------ --------------------------------------------------------------------------- 
  88     Call to deprecated method getTree() of class CRM_Core_BAO_CustomGroup:     
         Function demonstrates just how bad code can get from 20 years of entropy.  
 ------ --------------------------------------------------------------------------- 

Blocker for stable version 1.0.

There should be means for "soft-deleting" ECK entities (a "trash bin" as exists for CiviCRM contact entities).

Reference:

• \Civi\Api4\Generic\Traits\SoftDeleteEntity
• \CRM_Contact_BAO_Contact::deleteContact()
• \CRM_Contact_BAO_Contact::contactTrash()

... making ECK entities incompatible with e. g. SearchKit's JOINs, see civicrm/civicrm-core#26192

• relevant Entity.get in https://github.com/civicrm/civicrm-core/blob/dc7bb4a899ea74044df0b8afd8a43b027da40732/ext/search_kit/Civi/Search/Admin.php#L133-L139
• relevant "dao" property switch in https://github.com/civicrm/civicrm-core/blob/dc7bb4a899ea74044df0b8afd8a43b027da40732/ext/search_kit/Civi/Search/Admin.php#L270
• code not accessed due to this issue: https://github.com/civicrm/civicrm-core/blob/dc7bb4a899ea74044df0b8afd8a43b027da40732/ext/search_kit/Civi/Search/Admin.php#L374-L391

Steps to reproduce:

1. Create an ECK entity type
2. Add an Entity Reference custom field to it
3. Apply civicrm/civicrm-core#26192
4. Create a SearchKit search for the ECK entity
5. Try to add a JOIN for the entity type referenced in the custom field - it won't be available for joining

I'd like to contribute. I think we could streamline some of the code and push up some small patches to civicrm-core to make the virtual DAOs work better.
However, I'm not able to create a branch, and forking is disabled, so I'm unable to open pull-requests.

When I save a new entity through the SearchKit UI that's auto generated by ECK, it says it saves, but then nothing happens.

When I view the logs, all I get is:

[debug] Silently ignoring exception in Afform processGenericEntity call: DB_DataObject Error: insert:No table definition for civicrm_eck_keys

Debugging is turned on, but there's no backtrace

I'm running CiviCRM 5.73.2
ECK 1.0-beta10

• Analyse code regarding PHP 8 compatibility
• Fix deprecations found (to be specified)

Follow-up to #44 and #34.

Permissions should be checked when creating/editing ECK entities through Afforms.

A lot of core tables have these columns:

• created_id: Contact ID who created the record
• created_date: Auto-timestamp when record was created
• modified_date: Auto-timestamp when record was last updated

Would that be useful in ECK entities?
This is a good example of how to add those columns: civicrm/civicrm-core#19709

If ECK entities reference contacts (through an entity reference custom field of type Contact), the merger does not seem to find those references. ECK should implement hook_civicrm_merge() to support this.

@dontub since you've already looked into that, do you have anything to add?

Potential blocker for stable version 1.0

systopia-reference: 24496

With the new SearchKit Toolbar, a link for adding new ECK entities can be added to a SearchKit display. However, since the path of the Add form for ECK entities is dependent on the subtype, there would have to be as many Add links as there are subtypes for the entity type. The link that is being created with the current version includes the subtype ID of the first search result.

The ECK listings the extension is being shipped with does that in an Afform with a custom template for the dropdown button.

Is it possible to provide multiple such links for an entity type to be used in the SearchKit Toolbar in a generic way?

Ping @colemanw

... avoiding page loads and keeping the user on the entity type admin form.

There's the "View" path for an ECK entity (civicrm/eck/entity/view?reset=1&type=FooBar&id=4) and the "Edit" path (civicrm/eck/entity/edit/FooBar/One#?Eck_FooBar=4), and there's the "generic" entity path for the tabset which includes tabs for viewing and editing an ECK entity (civicrm/eck/entity?reset=1&type=FooBar&id=4). Neither of the specific paths ("View" and "Edit") The "Edit" path (pointing to the Afform) does not open in the tabset when opened in a separate tab (not in a modal).

We should make sure the tabset is there when loading either path.

Planned permissions:

• Create any Type ECK entity (e.g. "Create any Vehicle" or "Create any Fruit")
• Create Type of type Subtype ECK entity (e.g. "Create Vehicle of type Truck" or "Create Fruit of type Apple")
• View any Type ECK entity
• View Type of type Subtype ECK entity
• Edit any Type ECK entity
• Edit Type of type Subtype ECK entity
• Delete any Type ECK entity
• Delete Type of type Subtype ECK entity

Also, there should be permissions for administration (maybe not too granular), e.g.:

• Create ECK entity type
• View ECK entity types
• Edit ECK entity type
• Delete ECK entity type

or simply

• Administer ECK entity types

Those are quite a lot of permissions but having them seems reasonable for such a generic extension. This could however be configurable per entity type with options like:

• Provide distinct permissions for this entity type (falling back to generic "Create/View/Edit/Delete ECK entities without permission restriction" permissions)
• Provide distinct permissions per subtype (falling back to the any permissions for this entity type)

The APIv4 unit test is currently failing because it attempts to create 2 EckEntityTypes, and then create sub-types for both of them. As part of the test, it gives them both a sub-type of the same name.

That should certainly work; it shouldn't matter if different entities have sub-types with the same name. However, because this extension is re-using a single OptionGroup, a constraint is enforced by CiviCRM core which does not allow OptionValues in the same group to have the same value.

At first I thought it was a core bug and opened up civicrm/civicrm-core#22828, however I looked at how the grouping is used in other option groups like Case Status, and in that use-case grouping is either "open" or "closed", but regardless of grouping the values still need to be unique.

So I don't think that PR is mergeable, and I think we need to switch this extension to creating a new option group for every Eck entity type. We can use hook_civicrm_managed to do it automatically.

Since upgrading to 5.69, a fatal error is thrown after clearing the cache with cv flush. Subsequent requests don't produce this error. It seems to be related to a custom field I have on activities pointing at an ECK entity.

Here's the error and stack trace:

The website encountered an unexpected error. Try again later.

TypeError: CRM_Core_DAO_AllCoreTables::getTableForEntityName(): Return value must be of type string, null returned in CRM_Core_DAO_AllCoreTables::getTableForEntityName() (line 365 of /var/www/html/vendor/civicrm/civicrm-core/CRM/Core/DAO/AllCoreTables.php).
Civi\Api4\Service\Schema\SchemaMapBuilder::getTableName('Eck_Campaign') (Line: 149)
Civi\Api4\Service\Schema\SchemaMapBuilder->addCustomFields(Object, Object, 'Activity') (Line: 74)
Civi\Api4\Service\Schema\SchemaMapBuilder->loadTables(Object) (Line: 52)
Civi\Api4\Service\Schema\SchemaMapBuilder->build() (Line: 309)
Civi\Api4\Utils\CoreUtil::getSchemaMap() (Line: 773)
Civi\Api4\Query\Api4SelectQuery->autoJoinFK('dashboard_contact.id') (Line: 183)
Civi\Api4\Query\Api4SelectQuery->buildSelectClause() (Line: 79)
Civi\Api4\Query\Api4Query->getSql() (Line: 90)
Civi\Api4\Query\Api4Query->getResults() (Line: 106)
Civi\Api4\Query\Api4SelectQuery->run() (Line: 107)
Civi\Api4\Generic\DAOGetAction->getObjects(Object) (Line: 94)
Civi\Api4\Generic\DAOGetAction->_run(Object) (Line: 72)
Civi\Api4\Provider\ActionObjectProvider->invoke(Object) (Line: 156)
Civi\API\Kernel->runRequest(Object) (Line: 256)
Civi\Api4\Generic\AbstractAction->execute() (Line: 91)
civicrm_api4('Dashboard', 'get', Array) (Line: 69)
CRM_Core_BAO_Dashboard::getContactDashlets() (Line: 46)
CRM_Contact_Page_DashBoard->run(Array, NULL) (Line: 322)
CRM_Core_Invoke::runItem(Array) (Line: 69)
CRM_Core_Invoke::_invoke(Array) (Line: 36)
CRM_Core_Invoke::invoke(Array) (Line: 88)
Drupal\civicrm\Civicrm->invoke(Array) (Line: 83)
Drupal\civicrm\Controller\CivicrmController->main(Array, '')
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 627)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 704)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

... in order to allow selecting ECK entities in the new Entity Reference custom field type developed in civicrm/civicrm-core#25471.

An EckEntityType can have any string as the name, which could be a problem.

Consider this scenario:

1. Create an EckEntityType with 'name' => 'EntityType'.
2. This will create a virtual API entity named EckEntiyType
3. Oops, that name is already taken.

In core, we have the same problem with virtual APIv4 entities created from custom field sets. The problem is solved by adding an underscore. So every custom virtual entity begins with Custom_. If we employed that solution here, then an eckType named EntityType would create a virtual entity named Eck_EntiyType and there would be no name conflict.

With the latest version of ECK on CiviCRM 5.69.4, create/update/delete actions are failing due to table_name being blank in the generated SQL. I think this might be related to #104, but I haven't done much digging yet.

This is the error I get when attempting to create an ECK record via the API4 explorer:

{
    "error_code": 0,
    "error_message": "DB Error: unknown error",
    "debug": {
        "info": "INSERT INTO `` (`title` , `subtype` , `created_id` , `modified_id` , `created_date` , `modified_date` ) VALUES ('test 1234' , '2' ,  2 ,  2 ,  20240205083213 ,  20240205083213 )  [nativecode=1103 ** Incorrect table name '']",
        "db_error": "unknown error",
        "sql": [
            "INSERT INTO `` (`title` , `subtype` , `created_id` , `modified_id` , `created_date` , `modified_date` ) VALUES ('test 1234' , '2' ,  2 ,  2 ,  20240205083213 ,  20240205083213 )  [nativecode=1103 ** Incorrect table name '']"
        ]   
    }
}

I'll test on a fresh CiviCRM install and make sure this is reproducible.

Trying to install ECK on Civi 5.57.3, I get the following error messages.

> cv ext:download 'de.systopia.eck@https://github.com/systopia/de.systopia.eck/archive/refs/tags/1.0-alpha7.zip'
Using extension feed "https://civicrm.org/extdir/ver=5.57.3|uf=WordPress|status=stable|ready=ready"
Downloading extension "de.systopia.eck" (https://github.com/systopia/de.systopia.eck/archive/refs/tags/1.0-alpha7.zip)

In Mapper.php line 588:

  Class "CRM_Eck_Upgrader" not found

Despite this error message, I am able to continue installing the extension. However, in the UI I get a message saying that extension database upgrades are required. On the extension database upgrade screen, this error message appears:

[Error: Upgrade de.systopia.eck to revision 0010]
invalid criteria for IN

Hi just wondering if I am doing something wrong as I cannot see any buttons on the ECK screen.
CiviCRM 5.67.1
MySQL8
Wordpress 6.4.1

Downloaded latest extension from github.

The screen displays and seems to try and display a second screen below it (which I think might be the subtype form?)

Any ideas are welcomed!

I'm experimenting with this and the new EntityRef field and it's brilliant. This is going to be so helpful! Thank you for all your work here.

I've hit a slight roadblock when I try to create a new record via api4. I get:

Mandatory values missing from Api4 Eck_CommsChannel::create: subtype

My eck entity doesn't have any subtypes - should it?

My code is:

$results = \Civi\Api4\EckEntity::create('CommsChannel', FALSE)
  ->addValue('title', 'E-bulletin')
  ->execute();

When upgrading CiviCRM Core from 5.58 to 5.61, I got this error thrown:

DROP TRIGGER IF EXISTS civicrm_eck_-extension_before_insert [nativecode=1064 ** You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '-extension_before_insert' at line 1]

Seems - is not allowed in trigger names, but the ECK entity table contains one.

We have been using ECK with the new entity reference field and are exploring how we can create many to many relationships. For example lets imagine we want to allow cases to be associated with one or more events (and visa versa). What we want to happen is when you select events as the entity in searchkit, you can then be able to select cases as a 'with' entity.

For a many to many in SQL you would typically create a table with two FKs that contains the ids of each entity. You can do something like this in ECK by creating a new entity (lets call is CaseEvent) and then adding two entity reference custom data fields to that table. But searchkit doesn't like this.

Having chatted with coleman, we think the two things that SK doesn't like are:

1. the entity is not registered as a bridge entity
2. the FKs are on attached to custom data, not the original entity.

Thinking about how to approach solving this in ECK, we could (thanks @colemanw for your ideas)

1. Introduce a "type" to the ECK entity that includes "Regular Entity" and "Bridge Entity"
2. When creating a bridge entity, the user would need to select 2 entities to bridge. This would add an extra couple of fields to the entity
3. It might also make sense to allow them to select between "one" and "many" for each side of the bridge (so you could create a "one to one" bridge, a "one to many" bridge or a "many to many" bridge) - need to think some more on

Also from coleman:

Note: last time I checked, APIv4/SK are a little buggy if both entities are the same (e.g. a bridge between "Activity" and "Activity" may confuse the API).

(untested) it may be possible to keep ECK simple and use custom fields instead of adding all the schema handling logic in ECK. In that case, the custom value table itself would be the bridge (and the eck table would be useless, so, bleh).

One final thought from me: title and subtype often don't make sense for bridge entities. we should consider making them optional in this case.

Unit tests are going to be very important for an extension such as this.
SearchKit provides a good model for adding APIv4-based tests in an extension.
I'm pretty sure they can be run without a CI server using GitHub actions.

Now that the tests are running via GitHub workflows, there's a test failing that used to not fail … @colemanw could you have a look when you get a chance?

Failing job: https://github.com/systopia/de.systopia.eck/actions/runs/7886176894/job/21518883580

Output:

1) api\v4\EckEntity\EckEntityTest::testTwoEntityTypes
Civi\Core\Exception\DBQueryException: DB Error: unknown error

/var/www/html/sites/all/modules/civicrm/CRM/Core/Error.php:971
/var/www/html/sites/all/modules/civicrm/vendor/pear/pear-core-minimal/src/PEAR.php:945
/var/www/html/sites/all/modules/civicrm/vendor/pear/db/DB.php:997
/var/www/html/sites/all/modules/civicrm/vendor/pear/pear-core-minimal/src/PEAR.php:575
/var/www/html/sites/all/modules/civicrm/vendor/pear/pear-core-minimal/src/PEAR.php:[22](https://github.com/systopia/de.systopia.eck/actions/runs/7886176894/job/21518883580#step:6:23)3
/var/www/html/sites/all/modules/civicrm/vendor/pear/db/DB/common.php:1927
/var/www/html/sites/all/modules/civicrm/vendor/pear/db/DB/mysqli.php:941
/var/www/html/sites/all/modules/civicrm/vendor/pear/db/DB/mysqli.php:413
/var/www/html/sites/all/modules/civicrm/vendor/pear/db/DB/common.php:1[23](https://github.com/systopia/de.systopia.eck/actions/runs/7886176894/job/21518883580#step:6:24)4
/var/www/html/sites/all/modules/civicrm/packages/DB/DataObject.php:2696
/var/www/html/sites/all/modules/civicrm/packages/DB/DataObject.php:1829
/var/www/html/sites/all/modules/civicrm/CRM/Core/DAO.php:487
/var/www/html/sites/all/modules/civicrm/CRM/Core/DAO.php:1681
/var/www/html/sites/all/modules/civicrm/Civi/Api4/Query/Api4Query.php:92
/var/www/html/sites/all/modules/civicrm/Civi/Api4/Query/Api4SelectQuery.php:106
/var/www/html/sites/all/modules/civicrm/Civi/Api4/Generic/DAOGetAction.php:107
/var/www/html/sites/all/modules/civicrm/Civi/Api4/Generic/DAOGetAction.php:94
/var/www/html/sites/all/modules/civicrm/Civi/Api4/Provider/ActionObjectProvider.php:72
/var/www/html/sites/all/modules/civicrm/Civi/API/Kernel.php:156
/var/www/html/sites/all/modules/civicrm/Civi/Api4/Generic/AbstractAction.php:[25](https://github.com/systopia/de.systopia.eck/actions/runs/7886176894/job/21518883580#step:6:26)6
/var/www/html/sites/default/files/civicrm/ext/de.systopia.eck/tests/phpunit/api/v4/EckEntity/EckEntityTest.php:118
/var/www/html/sites/default/files/civicrm/ext/de.systopia.eck/tools/phpunit/vendor/bin/simple-phpunit:119

Code failing:

CiviCRM's recommended coding practices have changed in the past few years, and this extension was written with a few outdated conventions which would be good to update.

• Don't use APIv3: Most new extensions have minimal or no support for APIv3. The newest ones, Afform and SearchKit, do not provide any APIv3 support at all. According to the Developer Docs, "v4 is recommended for all new projects". Since this is a new extension, any code needing to access EckEntity APIs would necessarily be a new project. Dropping support for v3 now would reduce maintenance costs for this extension and prevent integrators from adding technical debt by requiring them to use v4.
• Use BAO::writeRecord() and hooks: We are moving away from writing create() and del() functions in BAOs, instead allowing them to inherit writeRecord() and deleteRecord() from CRM_Core_DAO. Any pre/post processing belongs in hooks.
• Keep business logic out of the form layer: Per the above point, all business logic related to creating, updating and deleting records should be done by the BAO, preferably using hooks. Form post process functions should ideally do no processing at all, and hand it over to the BAO or API. In this extension, a mistake was made by calling CRM_Eck_BAO_EckEntityType::ensureEntityType() from CRM_Eck_Form_EntityType::postProcess - this causes the form to work correctly but the API to be broken.

Currently the menu item is shown to all users. My understanding is that until per-entity permissions are implemented, only users that can administer CiviCRM can view/edit custom entities.

I think it'd make sense to change the permissions of the menu item to align with the permissions of the entities (i.e., if my assumption about entity permissions is correct, change the permission on the menu item to administer CiviCRM). If not, would it be possible to surface the menu items under civicrm/admin/menu so that they can be customised?

I'm happy to submit a PR – just wanted to clarify the approach first 😄

While refactoring, I noticed that the APIv4 actions on the Eck_* API entities require different formats of the ECK entity type parameter depending on whether the action is being called in the procedural (civicrm_api4()) or OOP style (e.g. \Civi\Api4\EckEntity::get()).

The procedural form takes the API entity name, which is always prefixed with Eck_ for ECK entities. The OOP form takes the ECK entity type name without the Eck_ prefix, as all of the ECK-specific actions add that prefix internally when instantiating the action object.

I'm not sure this is clever, as converting code from procedural to OOP is error-prone, and it does not align with \Civi\Api4\Generic\AbstractAction::__construct() which takes the API entity name as a first parameter.

@colemanw do you think it's worth changing that to requiring the API entity name for OOP-style calls? Of course, this would have to add a BC layer until version 2.0.0 of ECK …

There's no UI for creating/editing ECK entities yet. We originally had Afform forms in mind for providing such forms. Now that APIv4 is fully implemented, this should be doable.

For Afform routes to be picked up by the menu router, a UF cache flush is needed after adding/editing an ECK entity type, as otherwise the entity listings for those types would return a 404 Not Found.

I noticed the \CRM_Eck_BAO_EckEntityType::ensureEntityType() function takes a lot of trouble to handle the possibility of renaming an entity type. Not just changing the label (which is easy) but actually changing the entity name and even the sql table name!

Note the precedent in CiviCRM core:

• Custom Groups and Custom Fields cannot be renamed
• Activity types cannot be renamed
• Contact types cannot be renamed
• Relationship types cannot be renamed
• Case types cannot be renamed

IMO renaming entity types should not be allowed, and the API should throw an exception if attempted. If it is allowed, very thorough unit test coverage should be added, checking complete data integrity after a rename operation.

... because otherwise Symfony complains with Cannot use UTF-8 route patterns without setting the "utf8" option for route "/civicrm/eck/entity/edit/Süperentity/Bädsubtypename/{extra}"

We should do one of the following:

• Allow routes to contain UTF-8 characters (if that is at all possible with Afform/SearchKit - @colemanw?)
• Validate/transliterate Entity type and subtype names (their labels can still contain UTF-8)
• Replace non-ASCII characters with a placeholder (e.g. _) in relevant places (entity type name, subtype name)
• Use the sub types' value instead of their name for Afform names and routes