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

[PHP] Improve validation on empty arrays #7686

Merged
merged 2 commits into from
Feb 22, 2018

Conversation

ackintosh
Copy link
Contributor

@ackintosh ackintosh commented Feb 19, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

refs #7675 (comment)

PetApi::findPetsByStatus() should throw an exception if an empty array is passed.

@ackintosh
Copy link
Contributor Author

@wing328
Copy link
Contributor

wing328 commented Feb 22, 2018

@ackintosh The enhancement to check for empty array looks good to me 👍

@wing328 wing328 merged commit 14e1e19 into swagger-api:master Feb 22, 2018
@ackintosh
Copy link
Contributor Author

👀 ! Thanks, I'll do that.

@ackintosh ackintosh deleted the validate-empty-array branch February 24, 2018 03:10
@ackintosh
Copy link
Contributor Author

ackintosh commented Feb 24, 2018

Ah, the check for empty array in model may not needed. Because:

  • model takes a single value (not multiple)
  • passed value to model will be checked here whether it's valid

So empty array is always detected as an invalid value.


While I was investigating the above, I realized two different problems. 💡

1. We should pass true as 3rd argument of in_array()

        $enum = new EnumTest([
            'enum_string'  => 0, // Invalid
            'enum_integer' => 1,
            'enum_number'  => 1.1,
        ]);

        var_dump($enum->listInvalidProperties());
        // array(0) {
        // }

        var_dump($enum->valid());
        // true

It is caused by in_array() without 3rd argument.

2. Non-required enum property is listed as invalid when omitted

        $enum = new EnumTest([
            'enum_string'  => 'UPPER',
            // 'enum_integer' => 1, Omit non-required property
            'enum_number'  => 1.1,
        ]);

        var_dump($enum->listInvalidProperties());
        // array(1) {
        //   [0] =>
        //   string(58) "invalid value for 'enum_integer', must be one of '1', '-1'"
        // }

        var_dump($enum->valid());
        // bool(false)

enum_integer is non-required property, but it is listed as invalid.

ackintosh added a commit to ackintosh/swagger-codegen that referenced this pull request Feb 25, 2018
refs swagger-api#7686 (comment)
> 2. Non-required enum property is listed as invalid when omitted
@ackintosh ackintosh mentioned this pull request Feb 25, 2018
4 tasks
ackintosh added a commit to ackintosh/swagger-codegen that referenced this pull request Feb 25, 2018
refs swagger-api#7686 (comment)
> 1. We should pass true as 3rd argument of in_array()
wing328 pushed a commit that referenced this pull request Mar 8, 2018
* Add required enum property

* Update samples

* Add test case which reproduce the problem

refs #7686 (comment)
> 2. Non-required enum property is listed as invalid when omitted

* If the property is not empty, perform validation

* Update samples

* Use is_null() according to setter implementation

refs https://github.com/ackintosh/swagger-codegen/blob/377247f125988b591685e9f7cedc95fe5e99bcc2/modules/swagger-codegen/src/main/resources/php/model_generic.mustache#L347

* Update samples
wing328 pushed a commit that referenced this pull request Mar 25, 2018
* Add test case which reproduce the problem

refs #7686 (comment)
> 1. We should pass true as 3rd argument of in_array()

* Add test case for setter

* Strict validation

* Update samples

* Tweak expected value according to changes in #7723
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants