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

Invalid invalid type handling #19

Closed
scaytrase opened this issue May 16, 2019 · 7 comments · Fixed by #21
Closed

Invalid invalid type handling #19

scaytrase opened this issue May 16, 2019 · 7 comments · Fixed by #21

Comments

@scaytrase
Copy link
Contributor

If data type does not match schema code still tries to utilize it as a scalar value. moreother it tries utilize value itself rather than type, and this can lead to huge error texts

notice: "Array to string conversion in /data/www/content.leos.lamoda.ru/vendor/lezhnev74/openapi-psr7-validator/src/Schema/Keywords/Type.php on line 81"

I'll try to fix it

@scaytrase
Copy link
Contributor Author

Also some questions @lezhnev74

if (! isset($this->parentSchema->items)) {

This line should some kind of schema validation step, not data validation. It's rather strange to decline data here if schema is invalid. Probably this should go to cebe/php-openapi instead

if (count($data) && ! (is_array($data) && ArrayHelper::isAssoc($data))) {

counting data befor validating its type is unsafe (especially for php 7.2). why we expecting items in data at all?

@scaytrase
Copy link
Contributor Author

Also schema validation is duplicated here

])->stringType()->assert($type);

and here

throw new Exception("Type '%s' is unexpected", $type);

I hope we can leave second only, it's more obvious

@lezhnev74
Copy link
Owner

lezhnev74 commented May 16, 2019

Great points!

  1. $this->parentSchema->items.
    Agree, this is too defensive. This should be delegated to the underlying package.
  2. count($data) && ! (is_array($data) && ArrayHelper::isAssoc($data))
    This must make sure that the data is an array, and it has no string keys (which makes it a dictionary in terms of JSON). What would be a better check here? Ah, I jsut saw the PR. This code solves the problem: if (! is_array($data) || ArrayHelper::isAssoc($data)) {...
  3. Also schema validation is duplicated here.
    Agreed. Let's leave just switch statement here. Again, sometimes my code is over defensive.

@scaytrase
Copy link
Contributor Author

  1. everything but counting is ok. counting is unsafe and really not needed. to check vector against array is_array($data) && !ArrayHelper::isAssoc($data) is enough. Counting on non countable would fail in 7.2+

https://php.net/manual/en/migration72.incompatible.php#migration72.incompatible.warn-on-non-countable-types

@scaytrase
Copy link
Contributor Author

For 1 and 3 I can update #21 removing these additional checks

@lezhnev74
Copy link
Owner

Please, go ahead, sounds good!

@scaytrase
Copy link
Contributor Author

Done. Raised one more question in PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants