-
-
Notifications
You must be signed in to change notification settings - Fork 409
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[make:entity] fix multiple and nullable enums #1584
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 minor details and we should be all set!
if (null !== $mapping->enumType) { | ||
if ('array' === $typeHint) { | ||
// still need to add the use statement | ||
$this->addUseStatementIfNecessary($mapping->enumType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can eliminate the else
conditional in this if
statement by moving addUserStatement...()
outside and after `if ('array' === $typeHint) is called.
We're calling the add use statement method regardless if array
=== $typeHint
correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're calling the add use statement method regardless if
array
===$typeHint
correct?
We're calling it in both cases, but if array === $typeHint
, we don't want the returned string; it's just to automate the use statement.
} else { | ||
$typeHint = $this->addUseStatementIfNecessary($mapping->enumType); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity to my suggestion above, removing that use statement call and moving this one outside of the if
conditional would accomplish the same thing with less code if I'm not mistaken.
} else { | |
$typeHint = $this->addUseStatementIfNecessary($mapping->enumType); | |
} | |
} | |
$typeHint = $this->addUseStatementIfNecessary($mapping->enumType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will override $typeHint
with the enum type even if we use an array, which was the initial problem.
We don't want to use the returned type of addUseStatementIfNecessary
if we specify an array.
yield 'it_creates_a_new_class_with_enum_field_multiple_and_nullable' => [$this->createMakeEntityTest() | ||
->run(function (MakerTestRunner $runner) { | ||
$this->copyEntity($runner, 'Enum/Role-basic.php'); | ||
|
||
$runner->runMaker([ | ||
// entity class name | ||
'User', | ||
// add additional field | ||
'role', | ||
'enum', | ||
'App\\Entity\\Enum\\Role', | ||
// multiple | ||
'y', | ||
// nullable | ||
'y', | ||
// finish adding fields | ||
'', | ||
]); | ||
|
||
$this->runEntityTest($runner); | ||
}), | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this test is relevant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Fan2Shrek
Fixes #1580
Fix: No use statement when enum are multiple
The problem was due to the first condition
'array' === $typeHint && !$nullable
.If both "multiple" and "nullable" were selected,
$typeHint
was overridden by the second condition.