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] #126: Not-required properties now shows as nullable #129

Merged

Conversation

fMads
Copy link
Contributor

@fMads fMads commented May 22, 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: Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Fixed issue #126

Technical Committee

@fMads fMads force-pushed the mf_126_not_required_properties branch from c831558 to f906470 Compare May 22, 2018 06:49
@jimschubert
Copy link
Member

I know I'm not tagged in the technical committee on this, but I wasn't sure what generator(s) this affected based on the title.

I just wanted to comment that required in OAS 2.0 doesn't mean a value is nullable, rather that a property isn't required in the input/output structure (for example, in a PATCH operation). I'm sure many generators equate required to nullable, especially because OAS 2.0 is somewhat vague that it refers to structures and not data.

OAS 3.0 has a setting on Schema objects called nullable which is designed specifically for indicating properties are nullable. That is, you can have a required field with is or is not nullable. Similarly, you can have a non-required field which is or is not nullable. An example of a non-nullable non-required field is an int, where the value would be expected to be the default int value.

OAS 2.0/3.0 doc on required:

Determines whether this parameter is mandatory. If the parameter is in "path", this property is required and its value MUST be true. Otherwise, the property MAY be included and its default value is false.

OAS 3.x doc on nullable:

Allows sending a null value for the defined schema. Default value is false.

This isn't a blocker for this PR by any means, I just wanted to call it out as a consideration for templates and generators as the 3.0 spec gains traction.

@jmini jmini changed the title #126: Not-required properties now shows as nullable [php] #126: Not-required properties now shows as nullable May 24, 2018
@wing328 wing328 added this to the 3.0.0 milestone May 28, 2018
**capital_snake** | **string** | | [optional]
**sca_eth_flow_points** | **string** | | [optional]
**att_name** | **string** | Name of the pet | [optional]
**small_camel** | **object** | | [optional]
Copy link
Member

Choose a reason for hiding this comment

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

@fMads looks like something wrong. Not related to change in this PR. All type becomes "object".

Can you rebase this PR on the latest master (or merge the latest master into this PR), build the JAR (mvn clean package) and try to update the PHP samples again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - done :)

@fMads fMads force-pushed the mf_126_not_required_properties branch from b6f5302 to 11e7dfc Compare May 28, 2018 09:13
@wing328 wing328 merged commit 3beeb4e into OpenAPITools:master May 28, 2018
@wing328
Copy link
Member

wing328 commented May 28, 2018

@fMads FYI. I've updated the PHP samples via #169

@fMads
Copy link
Contributor Author

fMads commented May 29, 2018

@wing328 Thanks :)

@fMads fMads deleted the mf_126_not_required_properties branch May 29, 2018 07:04
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

4 participants