Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[OptionsResolver] Exception doesn't report the actual type used#11021
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[OptionsResolver] Exception doesn't report the actual type used#11021
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| $option, | ||
| $printableValue, | ||
| implode('", "', $allowedTypes) | ||
| implode('", "', $allowedTypes), | ||
| gettype($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.
This doesn't work nice with objects, its better to use something likeis_object($value) ? get_class($value) : ettype($value) so you get the class-name instead of object ;)
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.
Thanks, fixed
| @@ -253,7 +259,7 @@ private function validateOptionsExistence(array $options) | |||
| ksort($diff); | |||
| throw new InvalidOptionsException(sprintf( | |||
| (count($diff) > 1 ? 'The options "%s" do not exist.' : 'The option "%s" does not exist.').' Known options are: "%s"', | |||
| (count($diff) > 1 ? 'The options "%s" do not exist.' : 'The option "%s" does not exist.') .' Known options are: "%s"', | |||
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 should be reverted, Symfony CS doesn't use spaces around concatenating.
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.
Thanks, reverted.
As a side note, I couldn't find a convention about concatenation in theCS nor in theconventions.
Could it get added to (one of) those pages?
@cleentfaar Can you have a look at the comments and update this PR accordingly? |
@Tobion The only other option would be to abstract this into a component, but I find it quite overkill (I think@webmozart proposed to do that at some point as well.) |
@fabpot It could be worth considering we need this type of logic everywhere. And it's not implemented consistently yet. |
@fabpot Sorry, haven't had the time to get back on this PR lately. Thanks for the feedback guys, I'm fixing it right now! |
@webmozart Did you manage to get a component ready for this like@fabpot mentioned? If not I'm happy to get one set-up. Perhaps naming it something like Not sure how I would make the Thanks! |
@fabpot@webmozart Any feedback on my last comment? |
If you are talking about creating a new component, that's not required for this PR and it should be discussed before writing code. So, this PR is fine by me as it is now. |
| @@ -294,13 +300,14 @@ private function validateOptionsCompleteness(array $options) | |||
| private function validateOptionValues(array $options) | |||
| { | |||
| foreach ($this->allowedValues as $option => $allowedValues) { | |||
| if (isset($options[$option])) { | |||
| if (array_key_exists($option, $options)) { | |||
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.
is it intended to change the waynull values are handled ?
@cleentfaar Any news? |
@fabpot Sorry been away from this branch for a while. Thanks for the feedback guys, I've applied your comments and squashed everything down so it's ready for merging imo. Let me know if there's something left though. P.S. |
The components should not have such a dependency, which is why this code is duplicated everywhere. Also please rename the methods according to#10687: |
@Tobion I've updated the code again, replacing my own formatters with those of#10687 for sake of consistency. I can't really agree though on the (potentially) prettifying of |
ping@webmozart |
BTW, the test failure is not related to this PR |
| * | ||
| * @return string | ||
| */ | ||
| protected function formatTypeOf($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.
all these helper methods should be private
@cleentfaar Will you have time to update this PR according to@stof comments? |
this need to be rebased to fix conflicts with the huge refactoring done in#11716 |
2698bb3 toe4bff68Compare@stof Will fix this tonight, thanks! |
fba38bb to98c4accCompare…olverfurther improved message of the exception
88d2d61 to0de9a59Compare@stof Could you have a look again (rebased now)? I've left out the parts where you could pass the format as an argument, as the user could not never control these arguments, the methods are only used during exception. Hope this is ok. |
| @@ -228,7 +247,7 @@ public static function validateTypes(array $options, array $acceptedTypes) | |||
| continue; | |||
| } | |||
| $value = $options[$option]; | |||
| $value = $options[$option]; | |||
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 should be reverted.
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.
Cheers
| $printableValue, | ||
| implode('", "', $optionTypes) | ||
| static::formatValue($value), | ||
| static::formatTypesOf($optionTypes) |
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.
calling private methods must useself::, notstatic::, otherwise it is broken when extending the class, by trying to call it on the child class
| @@ -79,13 +98,13 @@ class Options implements \ArrayAccess, \Iterator, \Countable | |||
| public static function resolve(array $options, $defaults) | |||
| { | |||
| if (is_array($defaults)) { | |||
| static::validateNames($options, $defaults, true); | |||
| self::validateNames($options, $defaults, true); | |||
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.
validateNames() ispublic. Therefore,static must be kept here.
This is replaced by#12156. |
Note to moderator: Stopwatch tests are failing randomly here; not related to this PR!
For more info, see#10454
UPDATE:
UPDATE 2:
UPDATE 3: