[REF] CustomField code cleanup #16968
Conversation
|
(Standard links)
|
| 'AdvMulti-Select', | ||
| ]) | ||
| ) { | ||
| if (is_array($fieldValue) && $this->_fields[$name][$fieldName]['html_type'] == 'CheckBox') { |
There was a problem hiding this comment.
The history of this section of code is murky but from what I can tell, this is applying special formatting to checkboxes.
So instead of a double-negative conditional "not field types that are not checkbox" let's just say "if checkbox".
Perhaps @monishdeb can confirm this as the git history suggests he was involved writing this originally.
There was a problem hiding this comment.
so it looks like possible html types are
'CheckBox':
'Multi-Select':
'Multi-Select State/Province':
'Multi-Select Country':
'Select':
'Autocomplete-Select':
'Radio':
'Select Country':
'Select State/Province':
''Select Date'
'File'
'Link'
'Text Area'
'Text'
To validate this we need to confirm that only the top 4 could be arrays at this point
There was a problem hiding this comment.
@eileenmcnaughton so the two ways of writing and evaluating a refactor would be (roughly):
- The code analysis approach: Analyze the original code and figure out what it does based on all possible inputs. Make sure the refactored code behaves exactly the same in all circumstances.
- The "what were they thinking?" approach: Guess what the developer was trying to accomplish (which may not match what the code actually says because developers are only human). Make sure the refactored code satisfies their intentions.
I'm going with number 2, because based on the context (and basically the same code being duplicated in many other places) that this block is for the sake of checkboxes only, even though that's not exactly what it said.
|
retest this please |
The history of this section of code is murky but from what I can tell, this is applying special formatting to checkboxes. So instead of a double-negative conditional "not field types that are not checkbox" let's just say "if checkbox".
|
Tested, works fine. good to merge! |
|
I'm happy with this merge based on @pradpnayak's testing |
Overview
This is some initial code cleanup and refactoring to make CustomFields a bit more consistent.
I'm trying to standardize on using the
isSerializedmethod rather than ad-hoc guesswork based on the html_type.