-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for array of enums #9497
Conversation
This allows the use of 'array' and 'simple_array' in combination with the enumType parameter.
Thank you for your PR. As this is a new feature, please target the 2.12.x branch. Also, we need tests that cover your change. |
This is a great idea, i will comment on some code improvements later |
By the way, you can run all those complaining tools locally and get faster feedback. You don't have to push your changes through our CI everytime. Once you've installed the dependencies with
|
Cheers, think I've cleared them all now 🙂 |
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.
Initially I was wondering if the generic is_array
or else handling isn't a big "dangerous", because there is no connection to the Type
instance, where we know if value is an array or not, essentially allowing to "use this wrong".
But since getValue/setValue only happen to be used on data coming from the database, not from user input, I haven't found a simple example where this is not programmers error.
I am inclined to ignore that for now and see what is coming out of this change.
Very happy about the change, its simple addition allowing arrays of enums makes sense.
$e | ||
); | ||
|
||
if (is_array($value)) { |
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.
What do you think of refactoring it to have less duplication?
Yo could introduce a private helper method initializeEnumValue
that extracts what is currently in line 81-94 and 96-108. I believe its exactly the same code or did I miss something?
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.
The code is exactly the same yeah. I've added the helper in 01a1b75.
The only thing I'm not sure about is the wording of '$item', but I couldn't come up with anything better.
How can we move forward here? The PR needs a rebase at least. |
I can't replicate the phpcs failure locally. |
I've pushed a commit that should fix it. |
Thank you @dreadnip! |
* 2.12.x: Add support for array of enums (doctrine#9497) explicitly use the non-deprecated ORMException
Allow the use of 'array' and 'simple_array' types in combination with the enumType attribute parameter.
This is my first PR to the project, any feedback or instructions are welcome.
I had to duplicate the try/catch which introduces a bit of redundancy. Should I make a new method for this?