Skip to content

Conversation

@toomuchpete
Copy link
Contributor

The default host will be automatically set on the client by the ApiClient constructor.

PR checklist

  • Read the contribution guildelines.
  • Ran the shell/batch 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)
  • Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

As reported in #4511, Calling setHost() is unnecessary because ApiClient() constructor configures itself based on the default. This PR simply removes that line.

Resulting clients should be equivalent at runtime.

The default host will be automatically set on the client by the ApiClient constructor.
@wing328
Copy link
Contributor

wing328 commented Jan 9, 2017

@toomuchpete thanks for the PR. Please also run ./bin/php-petstore.sh to update the Petstore sample so that the change can be verified by the CIs.

@wing328
Copy link
Contributor

wing328 commented Jan 9, 2017

@toomuchpete you may also want to add test case(s) to cover the issue moving forward: https://github.com/swagger-api/swagger-codegen/tree/master/samples/client/petstore/php/SwaggerClient-php/tests

@wing328
Copy link
Contributor

wing328 commented Jan 9, 2017

cc @arnested

@wing328 wing328 added this to the v2.2.2 milestone Jan 9, 2017
toomuchpete pushed a commit to toomuchpete/swagger-codegen that referenced this pull request Jan 10, 2017
toomuchpete pushed a commit to toomuchpete/swagger-codegen that referenced this pull request Jan 10, 2017
@toomuchpete
Copy link
Contributor Author

Hi @wing328, thanks for the feedback! I didn't include the petstore updates because they appeared to include more changes than just mine so I thought I might be doing the wrong thing.

I've pushed those changes up in two separate commits. The first contains the changes I expected to see, and the second has changes that, I think, are left over from other code changes.

I'll look into adding a test for this situation this week. Thank you for your patience with my first contribution. 😄

@wing328
Copy link
Contributor

wing328 commented Jan 11, 2017

@toomuchpete your first PR definitely looks good 👍

Please take your time and let us know if you need any help from us.

@toomuchpete toomuchpete force-pushed the php-clientgen-constructor-fix branch from 20e0996 to 96665de Compare January 11, 2017 20:46
@toomuchpete
Copy link
Contributor Author

Hello again, @wing328 -- happy to report that I added a test for this situation. I wasn't sure exactly where to put it, but the PetApiTest file seemed to have the other tests that were generic to all API classes, so I put it in there with them.

Let me know if there's anything else I can do to get this PR ready to be merged!

@wing328 wing328 merged commit 2e4de0c into swagger-api:master Jan 12, 2017
@wing328
Copy link
Contributor

wing328 commented Jan 12, 2017

@toomuchpete the PR with test cases look good to me. Thanks for your contribution 👍

@philip
Copy link

philip commented Jan 12, 2017

@wing328 @toomuchpete Looks good. I filed the same bug with a different PR, but your change is simpler and much preferred. FWIW, it was Issue #4540 and PR #4518. I tested your change and it works great. You even added a test case, what a good model citizen! :)

I also ran into the creating unrelated php-petstore.sh output and wondered, but it seems okay. Kind of your to commit those unrelated changes separately, though.

To summarize: now we can set host at runtime by either editing lib/Configuration.php or using getDefaultConfiguration()->setHost(), or we no longer have to instantiate ApiClient separately at runtime to do that. I see this as a low risk BC change, as I doubt many rely on the current behavior that sometimes ignores configuration settings. Most commonly, users that don't set host in the swagger file definition were affected by this bug.

@wing328 wing328 changed the title Remove unnecessary call to setHost() in the constructor [PHP] Remove unnecessary call to setHost() in the constructor Feb 20, 2017
davidgri pushed a commit to davidgri/swagger-codegen that referenced this pull request May 11, 2017
…4525)

* Remove unnecessary call to setHost() in the constructor

The default host will be automatically set on the client by the ApiClient constructor.

* Updated PHP API Classes corresponding to template updates in swagger-api#4525.

* Additional changes generated by the petstore update unrelated to swagger-api#4525, but seem to have not been included yet.

* Add test to prevent regressions of swagger-api#4525
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.

3 participants