Skip to content

[Php] Exclude query params when they're not required#12120

Merged
wing328 merged 15 commits intoOpenAPITools:masterfrom
ksvirkou-hubspot:feature/emptyQueryParamIfNotRequired
Apr 25, 2022
Merged

[Php] Exclude query params when they're not required#12120
wing328 merged 15 commits intoOpenAPITools:masterfrom
ksvirkou-hubspot:feature/emptyQueryParamIfNotRequired

Conversation

@ksvirkou-hubspot
Copy link
Contributor

@ksvirkou-hubspot ksvirkou-hubspot commented Apr 12, 2022

Exclude query params when they're not required

{
    "name": "archived",
    "in": "query",
    "description": "Whether to return only results that have been archived.",
    "required": false,
    "style": "form",
    "explode": true,
    "schema": {
        "type": "boolean",
        "default": false
    }
}

if archived is empty
https://api.*.com/crm/v3/objects/contacts

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@ksvirkou-hubspot
Copy link
Contributor Author

@wing328
Copy link
Member

wing328 commented Apr 13, 2022

Perform git status
HEAD detached at pull/12120/merge
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   samples/client/petstore/php/OpenAPIClient-php/lib/Api/FakeApi.php
	modified:   samples/client/petstore/typescript-fetch/builds/default-v3.0/models/UserType.ts

Reported by https://github.com/OpenAPITools/openapi-generator/runs/5992057816?check_suite_focus=true

For the typescript sample, it has been updated in the master. Please merge the latest master into your branch.

@ybelenko
Copy link
Contributor

@ksvirkou-hubspot could you add a few tests? You've added new $required argument to toQueryValue method, it should be covered.

@ksvirkou-hubspot
Copy link
Contributor Author

ksvirkou-hubspot commented Apr 13, 2022

[Php] Convert boolean value for query string #11880

Will do

@ksvirkou-hubspot
Copy link
Contributor Author

ksvirkou-hubspot commented Apr 13, 2022

I ve just fixed samples and added tests
Could you merge it, please?
@ybelenko @wing328

I guess PHPCodeSniffer would find PSR12 violated but we use CS-Fixer
instead. Anyway, conditions should contain spaces between logical
operators for readability.
Copy link
Contributor

@ybelenko ybelenko left a comment

Choose a reason for hiding this comment

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

@nadar does it look ok to you too? This changes might be related to deepObject serialization feature.

@atanasiuk-hubspot
Copy link
Contributor

Hey team, any updates on merging this?

@wing328 wing328 merged commit 6b3abd9 into OpenAPITools:master Apr 25, 2022
@wing328
Copy link
Member

wing328 commented Apr 25, 2022

Just merged. Thanks @ksvirkou-hubspot for the PR.

@ksvirkou-hubspot ksvirkou-hubspot deleted the feature/emptyQueryParamIfNotRequired branch April 25, 2022 10:27
@nadar
Copy link
Contributor

nadar commented Apr 26, 2022

Hey @ybelenko, sorry i never saw this one. But as along as it does not break your unit tests, this should not affect the deep object serialization.

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.

5 participants