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] [ci skip] Add nullable logic #3493

Closed
wants to merge 14 commits into from
Closed

[PHP] [ci skip] Add nullable logic #3493

wants to merge 14 commits into from

Conversation

githubERIK
Copy link

@githubERIK githubERIK commented Jul 29, 2019

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, ./bin/openapi3/{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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
    Master should be 🆗.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.
    @jebentier, @dkarlovi, @mandrean, @jfastnacht, @ackintosh, @ybelenko, @renepardon

Description of the PR

Fix #2199

How to check the outcome

<?php
require_once __DIR__ . '/vendor/autoload.php';
use Messente\Api\Configuration;
use GuzzleHttp\Client;
use Messente\Api\Api\ContactsApi;
use Messente\Api\Model\ContactUpdateFields;

$config = Configuration::getDefaultConfiguration();
$config->setUsername('____');
$config->setPassword('____');

$api = new ContactsApi(new Client(), $config);

try {
  $response = $api->createContact([
    'phoneNumber' => '+37251000000',
    'email' => '[email protected]',
    'firstName' => 'Any',
    'lastName' => 'One',
    'company' => 'Messente',
    'title' => 'Sir',
    'custom' => 'Any custom',
    'custom2' => 'Any custom two',
    'custom3' => 'Any custom three',
    'custom4' => 'Any custom four'
  ]);
  echo $response.PHP_EOL;
  echo '----'.PHP_EOL;

  $response = $api->fetchContact('+37251000000');
  echo $response.PHP_EOL;
  echo '----'.PHP_EOL;

  $updateFields = new ContactUpdateFields([
    'lastName' => 'Johnson',
    'custom2' => null,
  ]);
  $response = $api->updateContact('+37251000000', $updateFields);
  echo $response.PHP_EOL;
  echo '----'.PHP_EOL;

} catch (Exception $e) {
  echo 'Exception when calling the API: ', $e->getMessage(), PHP_EOL;
}
?>

and given "email", "firstName","lastName", "company", "title", "custom", "custom2","custom3", "custom4" are nullable:true check that ...

When a request goes out

  • field is nullable and value is null
    • ✅ when somebody set the value to null on purpose then this should not be filtered out by ObjectSerializer
    • ✅ when somebody did not specify the value when using the constructor (causing the constructor to automatically set the value to null) then that null value should be filtered out

When a response comes in

  • field is nullable and value is null
    • ✅ echo $response should show all null values.

@macjohnny
Copy link
Member

macjohnny commented Jul 29, 2019

thanks for your PR.
please add the technical committee and re-generate the samples

@githubERIK githubERIK changed the title Add nullable logic [PHP] Add nullable logic Jul 29, 2019
Provide non-nullable fields with responses
[skip ci]
- Fix non-v3 tests first
- Prefer setting fields nullable to providing example values
@githubERIK
Copy link
Author

The failing client tests show that null values are assigned to attributes.

  • For OAS3 examples, some fields can be declared as nullable:true to quickly get many tests passing.
  • For OAS2 exmples, fields must not be null.

@macjohnny, do you think that making sure that OAS2 examples are never null, is OK?
Or is there any way inside the templates to understand whether the generator is in OAS2 or OAS3 mode and make OAS2 fields always nullable?


Reference
Screenshot 2019-07-31 at 12 30 39

Helen of https://stackoverflow.com/a/48114322


More context
When I run

/Users/user/Documents/Apps/java/openapi-generator/bin/openapi3/php-petstore.sh

then isNullable is available in mustache templates and things work as a I expect.

However, when I run

/Users/user/Documents/Apps/java/openapi-generator/bin/php-petstore.sh

then the logic in mustache template

        {{#isNullable}}
        if (is_null(${{name}})) {
            array_push($this->openAPINullablesSetToNull, '{{name}}');
        } else {
            $nullablesSetToNull = $this->getOpenAPINullablesSetToNull();
            $index = array_search('{{name}}', $nullablesSetToNull);
            if ($index !== FALSE) {
                unset($nullablesSetToNull[$index]);
                $this->setOpenAPINullablesSetToNull($nullablesSetToNull);
            }
        }
        {{/isNullable}}

         {{^isNullable}}
        if (is_null(${{name}})) {
            throw new \InvalidArgumentException('non-nullable {{name}} cannot be null');
        }
        {{/isNullable}}

always leads to ^isNullable block, I think.

@macjohnny
Copy link
Member

I think we shouldn't change the OAS2 examples.
i guess there is a possibility to detect OAS2 in the template, e.g. you could add a variable in the controller which can be read in the template.
@wing328 do you know whether there already is such a variable somewhere?

@macjohnny
Copy link
Member

moreover, the default behavior should be to ignore null values to be assigned to non-nullable fields, in order to avoid breaking changes

@githubERIK githubERIK changed the title [PHP] Add nullable logic [PHP] [ci skip] Add nullable logic Aug 1, 2019
@githubERIK
Copy link
Author

githubERIK commented Aug 1, 2019

Created another pull request #3524 as well because one of the v3 tests wants to create an object which has something like const 0 = 0; but such code results in errors.

@githubERIK
Copy link
Author

githubERIK commented Aug 12, 2019

Somehow have to know whether OAS3 or OAS2 is being processed.

  • if the knowledge is available in template: conditionally use nullables or ignore them
  • if the knowledge is available in a place which calls PHP codegen: conditionally call PhpClientCodegen or PhpClientCodegenOas2 (does not exist currently).

I haven't found out how to get the info whether OAS3 or OAS2 document is being processed.

@wing328
Copy link
Member

wing328 commented Aug 22, 2019

For OAS2 examples, fields must not be null.

@githubERIK For OAS2, we can use x-nullable: true and {{#isNullable}} .. {{/isNullable}} will be set accordingly.

@ybelenko
Copy link
Contributor

Is this PR still relevant or should be closed without merge?

@arrilot
Copy link

arrilot commented Mar 8, 2021

This PR helped us a lot when we ported the code to the templates.
It looks like a working solution for at least OAS 3 and generator-cli: 4.3.1

@dennisameling
Copy link
Contributor

@githubERIK are you still planning to work on this? This issue is currently forcing us to build some workarounds for this issue. If you won't work on it anymore, I'm happy to create a new PR with your commits and get this pushed over the finish line!

@githubERIK
Copy link
Author

@githubERIK are you still planning to work on this? This issue is currently forcing us to build some workarounds for this issue. If you won't work on it anymore, I'm happy to create a new PR with your commits and get this pushed over the finish line!

@dennisameling, no, I'm not planning to work on this. The biggest obstacle for me was supporting OAS2 (#3493 (comment)); I got OAS3 case working and that was all I needed. It would be awesome if you could create a new PR (feel free to use my commits)! 👍

@dennisameling
Copy link
Contributor

It would be awesome if you could create a new PR (feel free to use my commits)! 👍

Alright, will try to have a look in the coming weeks! 😊

wing328 pushed a commit that referenced this pull request Jul 11, 2022
* [PHP] Add support to nullable (based on PR 3493)

* [AUTOGENERATED] update samples
@wing328
Copy link
Member

wing328 commented Jul 11, 2022

Closed via #12794

@wing328 wing328 closed this Jul 11, 2022
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.

[PHP] nullable attribute with value null gets removed
6 participants