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

Add DAO fields to API metadata when the entity type name is known #127

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

jensschuppe
Copy link
Collaborator

Follow-up to #125

Since the DAO methods for compiling schema fields are static, but ECK entities rely on the entity type for their metadata (table name, etc.), the CRM_Eck_DAO_Entity::fields() method needs to be called after initialization, and can't be relied upon for further calls for different entity types. #125 makes the method not return anything for uninitialized calls.

This PR uses the Spec Provider to gather DAO fields, as they would otherwise not be part of the entity metadata at all (causing e.g. search displays or even the entire SearchKit admin UI to crash).

@colemanw I had to copy a private method \Civi\Api4\Service\Spec\SpecGatherer::setDynamicFk() to save us from changes to Core. I tried to make this more generic, see civicrm/civicrm-core@master...jensschuppe:civicrm-core:daoSpec - this would have allowed for overridden getSupportedFields() methods in DAO classes that depend on the API entity name, but I'm not sure this would cover all relevant cases. And since DAOs are on their way out of Core anyway, I didn't file a PR to Core. Anyway, I'd still appreciate you reviewing.

@jensschuppe jensschuppe added bug Something isn't working status:needs review Code needs review and testing labels Apr 24, 2024
@jensschuppe jensschuppe added this to the 1.0 milestone Apr 24, 2024
@colemanw
Copy link
Collaborator

I think this is fine for now. But note that when the conversion in https://lab.civicrm.org/dev/core/-/issues/4999 is complete (and we are close!), then none of this will be needed. There won't be a need for DAO files at all.

@jensschuppe jensschuppe changed the title Add DAO fields when the entity type name is known Add DAO fields to API metadata when the entity type name is known Apr 25, 2024
@jensschuppe jensschuppe merged commit 734cf6a into master Apr 25, 2024
0 of 11 checks passed
@jensschuppe jensschuppe deleted the daoFieldSpec branch April 25, 2024 08:21
@jensschuppe jensschuppe added status:fixed The issue has been resolved (usually by committing/merging code) and removed status:needs review Code needs review and testing labels Apr 25, 2024
@jensschuppe
Copy link
Collaborator Author

Noting that this makes the listings of ECK entities (or any get call without where) now get an implicit filter for fields with default values (i.e. now for the created_date and modified_date fields) in https://github.com/civicrm/civicrm-core/blob/01dd6e55f61dfaa89d371b50a1aa456ead67dc99/Civi/Api4/Generic/AbstractGetAction.php#L42, which obviously doesn't make sense.

That was (accidentally) not a problem before, since the DAO did not return any fields. I guess we'll have to provide our own EckDAOGetAction and override that method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working status:fixed The issue has been resolved (usually by committing/merging code)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants