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

(REF) APIv4 FieldSpec - Extract various traits (Civi\Schema\Traits\*) #20875

Merged
merged 9 commits into from
Jul 19, 2021

Conversation

totten
Copy link
Member

@totten totten commented Jul 16, 2021

Overview

There are currently several different schemas floating around (e.g. for APIv3-fields, APIv4-fields, DAO-fields, and settings). These schemas have a mix of fields which are:

  1. The same (e.g. name, title, description).
    • We should ensure that these continue working the same in various schemas.
  2. Different in essential ways (e.g. SQL-mapped fields have table_name and column_name; setting-fields have group_name).
    • We should ensure that these can continue to be separate from each other.
  3. Different in non-essential ways (e.g. help_text vs help_pre/help_post; html vs input_type/input_attrs).
    • In the short-term, we should avoid further drift. In the long-term, we may want to refactor/consolidate.

Since these mixable sets of fields, it makes sense to track/document/audit them as traits.

Before

Civi/Api4/Service/Spec/FieldSpec.php has long list of metadata fields.

After

A majority of the metadata fields have been split-off into separate traits (Civi\Schema\Traits\*). If you need to model a new schema, you can re-use these traits -- ensuring that the names/types/descriptions match-up. They are:

  • BasicSpecTrait - Basic identification for the field. (Name, title, description)
  • DataTypeSpecTrait - Describe the type of data stored in a field. (Data-type, serialize, fk-entity)
  • OptionsSpecTrait - Describe allowed options. (Options, options-callback)
  • GuiSpecTrait - Describe how to display the field in a user-facing GUI. (Label, input-type, input-attrs, help)
  • SqlSpecTrait - Describe how to the field maps into SQL. (Table-name, column-name, sql-operators, sql-filters)
  • ArrayFormatTrait - Translate the spec into an array-format. (eg $field->defaultValue vs $field['default_value'])

Comments

This PR is a step toward introducing another schema for describing workflow message-template fields. However, that schema left as a separate issue.

Not sure there's a whole lot of QA'ing that makes sense beyond existing tests.

I did expand coverage a bit in ConformanceTest - this basically ensures that APIv4's getFields and the class-model in FieldSpec remain true to each other.

@civibot
Copy link

civibot bot commented Jul 16, 2021

(Standard links)

@civibot civibot bot added the master label Jul 16, 2021
@seamuslee001
Copy link
Contributor

Jenkins re test this please

@seamuslee001
Copy link
Contributor

This seems fine to me but will let @colemanw make a comment on it but I think this is a worth while thing and the unit test coverage should be pretty solid here

@seamuslee001 seamuslee001 added the merge ready PR will be merged after a few days if there are no objections label Jul 19, 2021
@colemanw
Copy link
Member

Agreed

@colemanw colemanw merged commit 180c1ce into civicrm:master Jul 19, 2021
@totten totten deleted the master-field-spec branch July 20, 2021 04:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-test 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