Skip to content
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

Check value is array before passing to DataValueDeserializer::newFromArray #28

Closed
wants to merge 1 commit into from

Conversation

filbertkm
Copy link
Contributor

It's possible that a value being handled in DataValueDeserializer::newFromArray is not an array, so we need to check.

Bug: T168681

@filbertkm
Copy link
Contributor Author

my guess is that the issue is a bot is misbehaving and passing bad input.

todo: add tests to cover this

@manicki
Copy link
Contributor

manicki commented Jun 23, 2017

I guess this might work, and I am happy to write tests etc if this is the way we want to go.

That said, looking at other data values classes than UnboundedQuantityValue which was related to the issue referred to above, it seems other classes provide less strict newFromArray, allowing all input and throwing an exception if the value is not an array (i.e. not the language-level error).
I am wondering whether we wouldn't like to to adjust UnboundedQuantityValue::newFromArray the same way. I filed wmde/Number#105 in case this was preferred.

DataValueDeserializer already handles the exception UnboundedQuantityValue::newFromArray would throw, so this should also solve the issue I believe.

Copy link
Contributor

@thiemowmde thiemowmde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh dear. This is not the correct fix. Look at StringValue, for example. It expects a string, not an array. We need to do bugfix releases of the geo and number components instead. I will explain in more detail in the Phabricator ticket.

@thiemowmde thiemowmde closed this Jun 23, 2017
@thiemowmde thiemowmde deleted the bug-T168681 branch June 23, 2017 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants