Check additionally for object values which result in empty values #61
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #61 +/- ##
========================================
Coverage 100% 100%
- Complexity 223 227 +4
========================================
Files 30 30
Lines 597 608 +11
========================================
+ Hits 597 608 +11
Continue to review full report at Codecov.
|
| */ | ||
| public static function checkForObjectType($value) | ||
| { | ||
| if (is_object($value)) { |
There was a problem hiding this comment.
Just for keeping the convention:
In this method you could return is_object($value) and add it as condition for UsergroupAwareSimpleValue::validate, as already is done for DataHelper::checkForEmptyValue.
E.g.
return DataHelper::checkForEmptyValue($value) && DataHelper::checkForObjectType($value);
There was a problem hiding this comment.
That might be a good idea.
Also, it would be better if the method checked if the value was an acceptable one (is_string() and is_numeric()) instead of checking if it was one of, possible multiple, disallowed types.
Another thing is: What if the passed object has a useful ::__toString() method that actually returns something that is worth being exported? If there was a nice way to check this, the library's convenience could be improved, but I guess it's hard to judge if a ::__toString() method makes sense at runtime. So we can probably expect users to explicitly call it.
| */ | ||
| public static function checkForObjectType($value) | ||
| { | ||
| if (is_object($value)) { |
There was a problem hiding this comment.
That might be a good idea.
Also, it would be better if the method checked if the value was an acceptable one (is_string() and is_numeric()) instead of checking if it was one of, possible multiple, disallowed types.
Another thing is: What if the passed object has a useful ::__toString() method that actually returns something that is worth being exported? If there was a nice way to check this, the library's convenience could be improved, but I guess it's hard to judge if a ::__toString() method makes sense at runtime. So we can probably expect users to explicitly call it.
| * @param mixed $value The value to check. | ||
| * @throw ObjectNotAllowedException In case the provided value is a object. | ||
| */ | ||
| public static function checkForObjectType($value) |
There was a problem hiding this comment.
Given that we change the check to check for permitted values instead of restricted ones, we should probably better call it something like checkForAcceptableType or just checkType.
| @@ -10,6 +10,14 @@ public function __construct($message = 'Empty values are not allowed!') | |||
| } | |||
| } | |||
|
|
|||
| class ObjectNotAllowedException extends \RuntimeException | |||
There was a problem hiding this comment.
Better naming would be something like UnsupportedTypeException or whatever better name you can think of, as we should change the check to check for acceptable values instead of one unacceptable one.
| @@ -10,6 +10,14 @@ public function __construct($message = 'Empty values are not allowed!') | |||
| } | |||
| } | |||
|
|
|||
| class ObjectNotAllowedException extends \RuntimeException | |||
| { | |||
| public function __construct($message = "Objects as values are not allowed!") | |||
There was a problem hiding this comment.
Don't forget to update the default message. Maybe pass the value as constructor parameter, so we can dynamically build a message like <VALUE TYPE> is not an acceptable export value..
|
@DomBra Thanks for taking a look as well! |
|
@mmachatschek Still interested in this PR? Just needs some issues being fixed and conflicts resolved. |
|
@howard Yes, still interested. But I'll probably will make another approach. I'll close the PR at the time I'll find the time for this. |
|
closed. Outdated. New PR following |
This fixes #59