Additional PHP8.1 hardening #31
Conversation
Uses the same logic as already included in EncryptedText
src/FieldType/EncryptedDatetime.php
Outdated
| @@ -42,6 +42,8 @@ public function setValue($value, $record = null, $markChanged = true) | |||
|
|
|||
| public function getDecryptedValue($value) | |||
| { | |||
| // Type hardening for PHP 8.1+ | |||
| $value = (string)$value; | |||
There was a problem hiding this comment.
Wouldn't it be better to require it to be a string from the start in the method calls? And require it to be a string
public function getdecryptedvalue(string $value = '') {}
There was a problem hiding this comment.
@Firesphere the getDecryptedValue functions are only ever called from the getValue function within the same class.
The only logic within the getValue functions are to return the getDecryptedValue function call.
Moving the type hardening from getDecryptedValue into getValue splits the logic across the 2 functions and raises the question of if the getDecryptedValue functions are necessary.
Is there any reason that the logic within getDecryptedValue could not be placed directly in getValue()?
There was a problem hiding this comment.
I'm curious why a simple hardening check (value isn't null, and if it is, make it an empty string), moving it to getValue() would suddenly "split the logic".
Given the method is public, hardening at call time instead of at execution time, makes more sense to me, as well as requiring a hardened call.
Further, at the getValue() point, if the value already is null, it is known that there is nothing to decrypt, thus even calling the method is somewhat pointless at that stage.
Unrelated further, the code duplication smells like it could be moved to a Trait to reduce the amount of copy-paste code.
src/FieldType/EncryptedDatetime.php
Outdated
| @@ -42,6 +42,8 @@ public function setValue($value, $record = null, $markChanged = true) | |||
|
|
|||
| public function getDecryptedValue($value) | |||
| { | |||
| // Type hardening for PHP 8.1+ | |||
| $value = (string)$value; | |||
There was a problem hiding this comment.
I'm curious why a simple hardening check (value isn't null, and if it is, make it an empty string), moving it to getValue() would suddenly "split the logic".
Given the method is public, hardening at call time instead of at execution time, makes more sense to me, as well as requiring a hardened call.
Further, at the getValue() point, if the value already is null, it is known that there is nothing to decrypt, thus even calling the method is somewhat pointless at that stage.
Unrelated further, the code duplication smells like it could be moved to a Trait to reduce the amount of copy-paste code.
Reduces duplication
|
@madmatt I'm pretty happy with the solution this way, and it reduces code smell. Any final thoughts from you¿ |
|
Looks good to me, thanks @scott-nz and @Firesphere! |
Uses the same logic as already included in EncryptedText.
NOTE: This is required for Silverstripe 4 and 5 so should be merged in before #30