Add APIv4 Batch.create spec #20501
Add APIv4 Batch.create spec #20501
Conversation
|
(Standard links)
|
| $spec->getFieldByName('created_date')->setDefaultValue('now'); | ||
| $spec->getFieldByName('modified_id')->setDefaultValue('user_contact_id'); | ||
| $spec->getFieldByName('modified_date')->setDefaultValue('now'); | ||
| $spec->getFieldByName('status_id')->setRequired(TRUE); |
There was a problem hiding this comment.
is there a reason why we aren't setting these to be required at the DB level?
There was a problem hiding this comment.
Yeah I found that it is set true at DB level as per schema here but in addition I think the API4 spec still need a declaration to indicate the field is required for validation and on API explorer UI as there are other such occurrences in core
civicrm-core$ grep -irn "setRequired(TRUE" Civi/Api4/Service/Spec/ | wc -l
23
There was a problem hiding this comment.
I would have thought that its only needed if the db isn't requiring it right @colemanw ?
There was a problem hiding this comment.
My bad, yes you are right. On further check it seems like the spec wrapper add the required property as per DAO::fields information. Updated the PR and removed the setRequired on status_id and title. Although I will submit a separate PR to remove the setRequired call on required (not pseudo) fields in other EntitySpecProvider files. Thanks for raising this.
monishdeb
force-pushed
the
api4_batch_spec
branch
from
44f6589 to
c5eed01
Compare
| $entity_table = new FieldSpec('entity_table', 'Batch', 'String'); | ||
| $entity_table->setTitle('Batch Entity Table - remove?'); | ||
| $spec->addFieldSpec($entity_table); |
There was a problem hiding this comment.
I don't understand what this extra field is for. I don't see any handling for an entity_table param in the CRM_Batch_BAO_Batch::create() function.
monishdeb
force-pushed
the
api4_batch_spec
branch
from
c5eed01 to
0b9986a
Compare
monishdeb
force-pushed
the
api4_batch_spec
branch
from
0b9986a to
4c35651
Compare
|
@eileenmcnaughton @colemanw on separate topic, is there a reason why essential field is not required in DB but required as per spec? Like civicrm_email.email is not required in DB but set required in spec : similarly civicrm_group.title, civicrm_phone.phone etc. |
|
I can't think of a good reason to not require those in the DB. It would be a small hassle to write the upgrade script so I guess no one has volunteered to do it. |
|
This PR looks complete. |
Overview
Add APIv4 Batch.create spec
Before
No default spec for APIv4 Batch.create spec
After
Add APIv4 Batch.create spec as declared here
Comments
ping @colemanw @eileenmcnaughton @seamuslee001
I think this was missed in #19931 !!!