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

Add controller.service_arguments tag to controllers to make them public #2059

Merged
merged 1 commit into from
Feb 9, 2019

Conversation

ossinkine
Copy link
Contributor

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 and ./bin/security/{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\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Fixes #1002
Continues #1003

cc @jebentier @dkarlovi @mandrean @jfastnacht @ackintosh @ybelenko @renepardon

@dkarlovi
Copy link
Contributor

dkarlovi commented Feb 5, 2019

Note, this obsoletes #1003 since it solves the problem in a cleaner way.

@wing328
Copy link
Member

wing328 commented Feb 6, 2019

@dkarlovi thanks for reviewing the change.

cc @Addvilz (the author of #1003) as well

@@ -25,7 +25,7 @@
"ext-mbstring": "*",
"symfony/validator": "*",
"jms/serializer-bundle": "*",
"symfony/framework-bundle": "^2.3|^3.0|^4.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of removing 2.3 I would bump it to 2.7. It should work as well, and there is a ton of 2.x projects out there still. Plus 2.7 is still receiving security updates. No need to break compat as long as it costs nothing.

Copy link
Contributor

@Addvilz Addvilz left a comment

Choose a reason for hiding this comment

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

Except for the version compat change (which is not a blocker), LGTM!

@dkarlovi
Copy link
Contributor

dkarlovi commented Feb 6, 2019

@Addvilz supporting a version of Symfony which is no longer supported by Symfony themselves is stretching this project too thin IMO. Of course, it would be best to support every version imaginable, but it's just not possible in most cases. We should only support officially supported versions, not EOL'd ones.

@Addvilz
Copy link
Contributor

Addvilz commented Feb 6, 2019

@dkarlovi Symfony 2.7 IS officially supported till May 2019. 2.8 till November 2019.

My argument is, supporting these versions costs absolutely nothing (e.g. there is no overhead on the project) AND they are still officially supported. Clearly no need to support 2.3 which is long extinct, but a bump to 2.7 would be advisable.

@dkarlovi
Copy link
Contributor

dkarlovi commented Feb 6, 2019

@Addvilz 2.7 / 2.8 is in security maintenance only so only critical security issues will get any attention there, they no longer receive bugfixes, let alone new features. I'm not sure we can call that "supported".

The tag we're adding came out with Symfony 3.3 (which is also EOL) so we cannot really support 2.x with this patch at all.

@Addvilz
Copy link
Contributor

Addvilz commented Feb 6, 2019

@dkarlovi it's not about what we call supported, it's what Symfony Release Process defines as supported. And security fix period falls under LTS support for versions I mentioned, and they are therefore supported. Users rely on these LTS support releases for that exact reason. I am aware of several large scale projects still in production on 2.8 specifically because 2.8 is LTS.

Versions prior to 3.3 are not affected by this issue at all and compatibility is also not affected. Adding a tag will have no effect on everything prior 3.3 as controllers were public by default and this issue simply did not exist.

Again, support for these versions costs nothing at all to OpenApi, so I don't see any reason to not support them while possible. There is no requirement for active support here and nor requirement to maintain backwards compatibility. I don't see semantics as a very good reason to break other peoples code if not doing so has literally no side effects.

@dkarlovi
Copy link
Contributor

dkarlovi commented Feb 6, 2019

Users rely on these LTS support releases for that exact reason. I am aware of several large scale projects still in production on 2.8 specifically because 2.8 is LTS.

Well, that's fair, they just use the older version of the generator?

Versions prior to 3.3 are not affected by this issue at all and compatibility is also not affected. Adding a tag will have no effect on everything prior 3.3 as controllers were public by default and this issue simply did not exist.

That's true. 👍

Again, support for these versions costs nothing at all to OpenApi, so I don't see any reason to not support them while possible.

The cost is any change, current or future, must consider all the supported versions. Supporting anything but the most recent versions always comes with a cost, which becomes higher the more elaborate your support is.

Having had experience with supporting 3 separate versions of Symfony in a quite used package where we wanted to introduce static code analysis: it's a nightmare.

I'd say a project core member must decide here what they wish to support. @wing328?

@dkarlovi
Copy link
Contributor

dkarlovi commented Feb 6, 2019

Thinking about this more, I agree with @Addvilz: this PR is not a place to do this change. Since it doesn't impact users of 2.x, we can keep it and just add the tag. Reducing the number of supported versions is a different problem which doesn't need to be solved here for this to be merged.

@ossinkine would you mind removing the composer changes?

@Addvilz
Copy link
Contributor

Addvilz commented Feb 6, 2019

@dkarlovi good point. Some time ago I was considering to rewrite large paths of this generator and/or create Symfony 4+ specific one maybe, and "legacy" this generator maybe. Unfortunately I am not spending so much time in PHP anymore so.

@ossinkine
Copy link
Contributor Author

I've removed composer changes

@wing328
Copy link
Member

wing328 commented Feb 8, 2019

I'd say a project core member must decide here what they wish to support. @wing328?

What about following Swift approach to create generators based on versions? e.g.

swift => swift 2.x (deprecated)
swift3 => swift 3.x
swift4 => swift 4.x

so we can keep the existing php-symfony generators for 2.x and 3.x. For 4.x, we create php-symfony4 instead similar to what Addvilz suggested

@wing328
Copy link
Member

wing328 commented Feb 8, 2019

If no further feedback on this PR, I'll merge it over the weekend.

@wing328 wing328 merged commit 8f5fa4d into OpenAPITools:master Feb 9, 2019
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
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.

4 participants