Fixes a bug where setting false value for attribute results in an empty string #457
Conversation
…t to an empty string. This is also true of boolean generator.
src/Generators/Base.php
Outdated
| @@ -117,7 +117,10 @@ public function getOptions() | |||
| */ | |||
| protected function getGenerator() | |||
| { | |||
| if ($this->kind === false) return $this->kind; | |||
There was a problem hiding this comment.
getGenerator() now has the incorrect return type. It is meant to only be string.
There was a problem hiding this comment.
Okay, where should this go, then? I think enforcing string here is either incorrect, or this check needs to be done outside this method. What do you suggest?
There was a problem hiding this comment.
Where is it documented that this could be set to false? The phpdoc for the kind and the constructor say only strings are allowed.
There was a problem hiding this comment.
As highlighted in #456, if I set an attribute's value to false as part of my test, it returns an empty string ' '. This breaks queries that have strict mode enabled, not to mention it's using a value that is unexpected.
Ignore my comment above - what would you suggest I do instead? I can update the Generic generator that checks for the value there, and if false simply returns. That's the only other location that it makes sense, I believe.
There was a problem hiding this comment.
Note that this also happens if you use a boolean generator and the generator returns false (instead of true), with strict mode enabled.
There was a problem hiding this comment.
My tests also pass (both on local project and FactoryMuffin), when moved to the Generic generate method.
There was a problem hiding this comment.
I think the correct usage would be to use the "callable generator" (in 3.0, in 2.1 is specific to only closures), and set the attribute to function () { return false; }.
Out of interest, why are you stuck on 2.1, rather than 3.0?
There was a problem hiding this comment.
Note that this also happens if you use a boolean generator and the generator returns false (instead of true), with strict mode enabled.
Oh. Hmm. I think I need to pull down the whole codebase and have a proper read over of the code again. Are you actually using 2.1? I'd rather only fix this in 3.x.
There was a problem hiding this comment.
Yeah that's a fair call. Tbh i didn't know 3.0 was available. I'll upgrade and then see if the issue still persists.
There was a problem hiding this comment.
Turns out upgrading to 3.0 is going to be an immense amount of work due to the move away from the facade and the faker changes. We may come back to this but for now I don't have the days it's going to take me to upgrade our test suite (we have 2300 tests, not including acceptance tests). Also not particularly interested in going through this if the issue still exists in 3.0.
| @@ -30,6 +30,10 @@ | |||
| */ | |||
| public function generate() | |||
| { | |||
| if ($this->kind === false) { | |||
There was a problem hiding this comment.
This still bothers me, since $this->kind has doc that says it's a string only.
There was a problem hiding this comment.
That's not true when you provide a true/false value, which is a valid argument as far as I can tell. Again, when using a boolean generator this still fails without this check. So the boolean generator itself isn't creating a true boolean - it's creating a string representation of a boolean. Imho, the design is incorrect OR we need a separate generator that deals purely with boolean values.
|
2.x series is EOL. |
Resolves issue #456.