[PHP] lib/Api constructors ignore configuration and set host to localhost

[philip]

Description

The constructors within lib/Api/*.php ignore the host definition in lib/Configuration.php, whether it's set within lib/Configuration.php itself or by using Swagger\Client\Configuration::getDefaultConfiguration()->setHost(https://example.org/); in the code.

Either way, the likes of $foo = new Swagger\Client\Api\Bar() always resets host to https://localhost.

Swagger-codegen version

2.2.1

Command line used for generation

swagger-codegen generate \
 --input-spec ./swagger.json \
 --lang php \
 --output /tmp/foo

Steps to reproduce

1. Use swagger_codegen to generate client, see above
2. Edit lib/Configuration.php by setting the $host there
3. Copy and edit example code from generated README.md
4. Optionally set host in code using Swagger\Client\Configuration::getDefaultConfiguration()->setHost('https://example.org/); -- generated code already does this for other stuff like username / password, which is why it's expected that this would work fine
5. Execute
6. Fails, as it attempts to connect to localhost instead of my desired host

Suggest a Fix

Code in the generated constructors looks like this:

        if ($apiClient == null) {
            $apiClient = new ApiClient();
            $apiClient->getConfig()->setHost('https://localhost');
        }

One option is to change it like so:

        if ($apiClient == null) {
            $apiClient = new ApiClient();
            if (empty($apiClient->getConfig()->getHost())) {
                $apiClient->getConfig()->setHost('https://localhost');
            }
        }

With that change, host can be set in lib/Configuration.php or within the code itself via the aforementioned setHost(), and not reset to localhost.

That said, I'm not familiar enough with the codebase to know if that's ideal, but it's working for me. But, defining the default host like this in multiple files feels odd so another solution is to define it somewhere else (like lib/Configuration.php) instead of within each constructor.

[philip]

Forgot to mention, an approach people must do now is pass in an API client, so something like:

$api_client = new Swagger\Client\ApiClient();
$foo = new Swagger\Client\Api\Bar($api_client);

That is a required step if you're not using https://localhost. I don't think it should be as the above code isn't much different if you're wanting to rely on lib/Configuration.php instead of passing in configuration options directly to ApiClient(). The above will use lib/Configuration.php values.

But if the current approach is fully intentional, then consider adding the ApiClient part in the generated PHP examples.

[wing328]

@philip thanks for reporting the issue. In your spec, did you set a proper host? e.g. https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/test/resources/2_0/petstore.yaml#L12

Ref: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#swagger-object

[philip]

@wing328 Ah, good question. We're encouraged to not define the host (here at work) as host depends on the user's setup / location. In other words, we have multiple hosts.

I'm not sure how common this is but do appreciate the information, as the current behavior makes more sense to me now. But, I still think it should be improved where changing the default host could be easier (one location) and have it always mind Configuration.php.

[wing328]

@philip given that the host setting depends on the user location, what about changing the default configuration instead during runtime?

https://github.com/swagger-api/swagger-codegen/blob/master/samples/client/petstore/php/SwaggerClient-php/lib/Configuration.php#L475-L499

[philip]

@wing328 I believe that's what this issue is about, isn't it? Today it can't be set at runtime that easily as setDefaultConfiguration's setHost() is ignored. The suggested fix makes it not ignored.

[wing328]

@philip may I know if you've time to submit a PR with test cases so that we can review the fix?

Test cases: https://github.com/swagger-api/swagger-codegen/tree/master/samples/client/petstore/php/SwaggerClient-php/tests

[philip]

@wing328 Yep, I'll give it a go hopefully this week.

[philip]

@wing328 I've submitted a PR, and also the php-petstore.sh results, but I'm not sure how to handle additional test cases yet. Perhaps others will have ideas in the meantime.

Regarding the php-petstore.sh results commit, it contained unrelated changes so perhaps others forgot to commit them in the past? Or I did something wrong?