Only set defaults when creating a custom field (not when editing one) #12240
Conversation
michaelmcandrew
force-pushed
the
dev/core#156
branch
3 times, most recently
from
0970701
to
8926ab5
Compare
CRM/Core/BAO/CustomField.php
Outdated
| $customField->is_active = CRM_Utils_Array::value('is_active', $params, TRUE); | ||
| $customField->is_view = CRM_Utils_Array::value('is_view', $params, FALSE); | ||
| if ($op == 'edit') { | ||
| $customField = CRM_Core_DAO_CustomField::findById($params['id']); |
There was a problem hiding this comment.
I don't get this - I understand that the params should not be applied by code in edit mode - but I don't think we need to worry about loading them so the DB doesn't apply them.
In general I think the preferred way to set defaults is at the api level. Unfortunately historically some form calls to the BAO::create rely on them being set in the BAO - but when done in the BAO the approach is generally to add to the $params in other BAO create functions - eg.
if ($op === 'create') {
$params = array_merge(['is_required' => 0, 'is_searchable' => 0...], $params);
}
But, let's do a quick audit of where create is called & see how much we really are relying on this anti-pattern of setting defaults in the BAO::Create anyway
There was a problem hiding this comment.
yeah, you are right :) I updated the title and the diff
michaelmcandrew
changed the title
michaelmcandrew
force-pushed
the
dev/core#156
branch
from
8926ab5
to
f2ff6ef
Compare
|
@eileenmcnaughton - this new diff takes into account your feedback though It is still using the |
| 'id' => $result['id'], | ||
| 'is_active' => 0, | ||
| ]; | ||
| $result = $this->callAPISuccess('CustomField', 'create', $params); |
There was a problem hiding this comment.
@michaelmcandrew looks like your not actually asserting anything other than the API works
What you probably want do to is add in $this->assertTrue($result['values'][$result['id']]['is_searchable'])
There was a problem hiding this comment.
@seamuslee001 - The test failed with an exception before the patch not applied. Just seeing it work was good enough for me. Though I take your point that I could be testing a bit more with the test.
|
test this please |
|
This makes sense - I'm Ok to merge it without worrying more about taking the tests further because I think we should try to get the SyntaxConformance ones working |
Before
When editing a custom field via the API, any values that were not supplied would revert to the default
After
When editing current values are loaded via the ->find() method.
See test for more details