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] By default should use static Configuration class instance #19775

Merged
merged 1 commit into from
Oct 5, 2024

Conversation

DarkSide666
Copy link
Contributor

@DarkSide666 DarkSide666 commented Oct 3, 2024

For PHP.

By default should use static Configuration class instance, otherwise new instance is created every time you make an API call.

Before this PR every time you create new API object and do not pass Configuration object as 2nd argument to API class constructor it creates new Configuration instance. This is wrong, because Configuration class is meant to be Singleton - so Configuration::getDefaultConfiguration() should be used instead.

Please review: @jebentier @dkarlovi @mandrean @jfastnacht @ybelenko @renepardon

@DarkSide666 DarkSide666 changed the title By default should use static Configuration class instance, otherwise new instance is created every time [PHP] By default should use static Configuration class instance Oct 3, 2024
@wing328
Copy link
Member

wing328 commented Oct 4, 2024

@DarkSide666
Copy link
Contributor Author

@wing328 could you please do this for me?
I'm on Windows and was trying to run generate_samples script from windows linux console (WSL), but still no luck.
It throws Error: Unable to access jarfile /mnt/c/wamp/www/DarkSide666/openapi-generator/modules/openapi-generator-cli/target/openapi-generator-cli.jar I guess this is not gonna work on Windows :(
Maybe I can just merge latest commits from your master branch into my PR and then it should be ok, isn't it?

@dkarlovi
Copy link
Contributor

dkarlovi commented Oct 4, 2024

I'm OK with this change, using static methods and properties is a code smell in 99% of the cases, but here the assumption is you should get the (supposedly pre-configured) default config instead of just an empty config object.

Not because of performance but because of the behavior, the solution is still the same. 👍

@DarkSide666
Copy link
Contributor Author

I'm OK with this change, using static methods and properties is a code smell in 99% of the cases, but here the assumption is you should get the (supposedly pre-configured) default config instead of just an empty config object.

Not because of performance but because of the behavior, the solution is still the same. 👍

Yes exactly. Normally because Configuration is static object, then it's meant to be pre-configured and then can be automatically be reused in all API calls done later in code.
But now you have to always do like this to use this "global" Configuration in your API classes:

$api = new \Example\Client\Api\PaymentsApi(
    null, // uses default \GuzzleHttp\Client()
    \Example\Client\Configuration::getDefaultConfiguration()
);

@wing328
Copy link
Member

wing328 commented Oct 4, 2024

no worry i'll update the sample over the weekend and get this PR merged

have a nice weekend

@wing328
Copy link
Member

wing328 commented Oct 5, 2024

tested updated samples locally and the result is good

PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

...............................................................  63 / 222 ( 28%)
............................................................... 126 / 222 ( 56%)
............................................................... 189 / 222 ( 85%)
.................................                               222 / 222 (100%)

Time: 00:01.512, Memory: 12.00 MB

OK (222 tests, 384 assertions)

will update the samples after merging this PR into master

thanks again for the PR

@wing328 wing328 merged commit 5e7cf1c into OpenAPITools:master Oct 5, 2024
14 of 15 checks passed
@wing328 wing328 modified the milestones: 7.8.0, 7.9.0 Oct 5, 2024
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