dhcp/kea: Refactor KeaDhcpv4.php and KeaDhcpv6.php to use BaseField helper methods #9450
Conversation
| $config = []; | ||
| $lexpireFields = iterator_to_array($this->lexpire->iterateItems()); | ||
| foreach ($lexpireFields as $fieldName => $fieldValue) { | ||
| $value = $fieldValue->__toString(); |
There was a problem hiding this comment.
This appears to be unused.
|
|
||
| public function isEnabled() | ||
| { | ||
| return (string)$this->general->enabled == '1' && !empty((string)(string)$this->general->interfaces); |
fichtner
left a comment
fichtner
left a comment
There was a problem hiding this comment.
ok with this but such refactors through the whole file should be taken with a grain of salt (risk for regressions)
I try (and sometimes fail) to avoid them when I'm not directly working on the code path.
|
I'll thoroughly test this before merging, it's not in any rush. I touched these files to improve consistency since thats something I noticed in this PR: #9401 |
| 'library' => '/usr/local/lib/kea/hooks/libdhcp_lease_cmds.so' | ||
| ]; | ||
| if (!empty((string)$this->ha->enabled)) { | ||
| if (!$this->ha->enabled->isEmpty()) { |
There was a problem hiding this comment.
Just out of curiosity (and for my open PR) is there a specific case where you'd want to use !(field->isEmpty()) vs field->isEqual('1') when checking if a boolean field is checked?
Because I've seen both being used and it's mildly confusing as to which is the preferred method for checking if a checkbox is checked.
There was a problem hiding this comment.
isEqual is a newer than isEmpty. isEqual(‘1’) or isEqual(1) is always safe, but isEqual(‘0’) had some issues in the past but is now always safe too. Though for booleans the best check to support perhaps would be isOn() and isOff() but that requires more brain smarts as it doesn’t really fit the base field. Just a tangent here since you asked 😊
|
Since I tested the before and after via these tests: #9455 I can say with some certainty that they will not cause huge regressions in config generation and other vital parts. |
No description provided.