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

[Symfony] Replace deprecated Controller with new one #2146

Merged
merged 1 commit into from
Mar 25, 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, ./bin/openapi3/{LANG}-petstore.sh, ./bin/security/{LANG}-petstore.sh and ./bin/openapi3/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

Symfony\Bundle\FrameworkBundle\Controller\Controller is deprecated and should be replaced with Symfony\Bundle\FrameworkBundle\Controller\AbstractController

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

@@ -25,7 +25,7 @@
"ext-mbstring": "*",
"symfony/validator": "*",
"jms/serializer-bundle": "^2.0",
"symfony/framework-bundle": "^2.3|^3.0|^4.1"
"symfony/framework-bundle": "^3.3|^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.

Again the same discussion with the same conclusion as we had before in #2059, so I guess 👍.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is the case when changing the version is justified.

@ossinkine ossinkine force-pushed the deprecated-controller branch from 4ef3f78 to b45453e Compare February 13, 2019 12:30
@Addvilz
Copy link
Contributor

Addvilz commented Feb 13, 2019

This is a welcome change but, I still suggest there should be a separate generator for Symfony 4+.

Navigating all of these compatibility issues (and the ones there will be in 4.x master) and maintaining compatibility between them is going to be rather nontrivial.

Controller class has been deprecated in 4.2 (3 months ago, see symfony/symfony#28243). So there is no rush with this either.

@ossinkine
Copy link
Contributor Author

ossinkine commented Feb 13, 2019

@Addvilz Just to understand your point. Symfony is updated quite often, the major version is released every two years, improvements and deprecations are every 6 months. Do you suggest to create a separate generator for each version of Symfony?

@Addvilz
Copy link
Contributor

Addvilz commented Feb 13, 2019

@ossinkine for each major version, it is not at all a bad idea. Considering the current generator was written to be compatible with 2.3 and php 5.4. Both of which are really badly end-of-life.

This would also make maintenance easier - a version reached EOL? Throw it in the trash. A new major version? Fork the previous generator, rewrite parts to work with the new major, and no need to maintain backwards compat with previous major releases.

With Symfony major releases there are generally quite a few major changes in structure etc. Warrants this kind of approach to me for sure.

@dkarlovi @wing328 wdyt?

@ossinkine
Copy link
Contributor Author

@wing328 If you approve the approach @Addvilz described, I'll prepare a new php-symfony4 generator.

@strobox
Copy link

strobox commented Feb 26, 2019

My 👍 for separate generator for symfony 4

@strobox
Copy link

strobox commented Feb 26, 2019

@ossinkine I've already done some bunch of adoptions for symfony. Fill free to get some ideas from them: strobox@c3a508c

@ossinkine
Copy link
Contributor Author

@strobox Thank you for your implementation. Why did you decide to remove JMS Serializer?

@strobox
Copy link

strobox commented Feb 28, 2019

@ossinkine I don't remember exactly. More likely that it was some conflicts with internal Symfony serializer.

@dkarlovi
Copy link
Contributor

Agree with creating a separate generator for Symfony 4, it would mean we could make it much better than the current one, basically deprecating the current one fully.

@wing328
Copy link
Member

wing328 commented Feb 28, 2019

Do you suggest to create a separate generator for each version of Symfony?

Yes, we can do that. Swift client generator is following this approach as we've swift, swift3 and swift4 generators and now only swift4 generator is actively maintained.

@dkarlovi
Copy link
Contributor

Do you suggest to create a separate generator for each version of Symfony?

Having a symfony4 generator would mean we could significantly improve it, with PHP 7.1 support (full type hinting), new quality assurance tools available, using Symfony's own serializer instead of JMS, all the latest best practices, without breaking compatibility.

@strobox
Copy link

strobox commented Feb 28, 2019

@ossinkine Obviously, my implementation can't pretend to be included to generator as whole. It also mix one not common feature: to serialize incoming requests bodies not to model defined in generators but to it superset classes that contains ORM/ODM mappings, if such provided. It is done through configuration of generated module in symfony project config (new file: Configuration.mustache
strobox@c3a508c#diff-df2c9e42aa5990c38d9ae33fdae8d4ab ):

# your_app\config\your_api_bundle_config.yaml
generated_api:
  models_overrides:
    'GeneratedApi\Model\PetModel': 'App\Entity\Pet'

Where PetModel is generated class and Pet is class+ORM mapping extending this model:
("modelNameSuffix": "Model" option added to openapi-generator config to add Model suffix to output class names)

/**
 * @ORM\Entity
 */
class Pet extends PetModel
{

I used descendant types as terminology in code for that approach.

So, my changes not applicable as stater point for new symfony4 generator implementation, because functionality and features was mixed with code that adopts generator to symfony 4. Will try to support new generator based on my experience.

@wing328 wing328 added this to the 4.0.0 milestone Mar 7, 2019
@wing328 wing328 changed the title Replace deprecated Controller with new one [Symfony] Replace deprecated Controller with new one Mar 7, 2019
@wing328
Copy link
Member

wing328 commented Mar 7, 2019

Shall I merge this PR into master for the time being while the PHP Symfony 4 generator is being worked on?

@wing328 wing328 merged commit 3677bdc into OpenAPITools:master Mar 25, 2019
@wing328
Copy link
Member

wing328 commented May 25, 2022

#11810 has been merged to support Symfony 6. Please give it a try and let us know if you've any feedback or question.

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