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-symfony] Symfony6 support #11810

Merged
merged 27 commits into from May 25, 2022
Merged

Conversation

BenjaminHae
Copy link
Contributor

Update php-symfony's generator template to work with Symfony6 and php8. fix #11322

This PR is based on #11339 and #10763.

I've explicitly decided not to create a new generator php-symfony6 as the current php-symfony generator only works with symfony 4.x which is under LTS support. It is very unlikely that symfony 4.xis used for new projects. Symfony 5.x is currently unsupported by openapi-generator.

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master (5.3.0), 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.
    @jebentier (2017/07), @dkarlovi (2017/07), @mandrean (2017/08), @jfastnacht (2017/09), @ackintosh (2017/09) ❤️, @ybelenko (2018/07), @renepardon (2018/12)

@BenjaminHae BenjaminHae mentioned this pull request Mar 18, 2022
5 tasks
@ybelenko

This comment was marked as resolved.

@BenjaminHae
Copy link
Contributor Author

The code generated by OpenAPI-generator is a package you can add as a dependency to an existing symfony project.
So the requirements (in composer.json) of the generated package do not have anything to do with the requirements of the symfony project - however they must be compatible.

One of the symfony dependencies required in the composer.json of the skeleton project you created has a dependency on jms/serializer-bundle so it is not noted in your composer.json as it is not a direct dependency of the skeleton project.

The normal workflow is (as described in the README created by the generator for php-symfony):

  1. Have an existing symfony project where you want to implement an API
  2. Generate the bundle using the OpenAPI generator
  3. require the bundle in the composer.json of your project
  4. Follow the README and implement the API functions within the symfony project

During this workflow you never touch the contents of the generated bundle.

@xvilo
Copy link
Contributor

xvilo commented Mar 19, 2022

Quicky looking at this, but I do have a couple of initiall remarks. I can do a proper review maybe this weekend, otherwise on Monday :)

@ybelenko
Copy link
Contributor

ybelenko commented Mar 19, 2022

The code generated by OpenAPI-generator is a package you can add as a dependency to an existing symfony project.

Ohh, of course. I forgot about that bundle/package driven behaviour in Symfony. Cool, then question is solved.

During this workflow you never touch the contents of the generated bundle.

That's too optimistic 🤣 🤣 🤣 Theoretically in perfect world - yes, but it always ends up so you need to overwrite something or add custom code across all current PHP generators.

@BenjaminHae
Copy link
Contributor Author

That's too optimistic 🤣 🤣 🤣 Theoretically in perfect world - yes, but it always ends up so you need to overwrite something or add custom code across all current PHP generators.

It works really well with my current project 😀

@ybelenko
Copy link
Contributor

That's too optimistic 🤣 🤣 🤣 Theoretically in perfect world - yes, but it always ends up so you need to overwrite something or add custom code across all current PHP generators.

It works really well with my current project 😀

Do you use polymorphism or other fancy features?) It also works great to me when I describe simple schemas of flat objects and basic CRUD operations.

*/
public function visitDiscriminatorMapProperty($data, ClassMetadata $metadata): string
{
return $this->jsonDeserializationVisitor->visitDiscriminatorMapProperty($data, $type);
Copy link
Contributor

Choose a reason for hiding this comment

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

$type does not exist in this method scope.

i guess you meant to use $metadata?

/**
* {@inheritdoc}
*/
public function startVisitingObject(ClassMetadata $metadata, object $object, array $type): void
Copy link
Contributor

Choose a reason for hiding this comment

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

2nd param changed from object $data to object $object compared to the base method

{
protected $jsonDeserializationVisitor;
Copy link
Contributor

Choose a reason for hiding this comment

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

type missing -> JsonDeserializationVisitor

/**
* @var int
*/
private $options = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd go with typed properties, rather than annotations

private int $options

private int $depth

@@ -19,22 +19,24 @@
}
],
"require": {
"php": "^7.1.3",
"php": ">=8.0.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be breaking to keep also >= 7.4 supported?
>=7.4
>=7.4.0|>=8.0.2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is: symfony6 only supports php>=8.0.2.
But now that I think about it, I can currently not see an obvious reason why symfony5 should not work.

I'll try it in the next days.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were no conflicts. So it works with symfony 5.

I'm not sure why I decided it needs to target symfony6 only in the first place.

@@ -5,18 +5,22 @@ namespace {{servicePackage}};
use JMS\Serializer\SerializerBuilder;
use JMS\Serializer\Naming\CamelCaseNamingStrategy;
use JMS\Serializer\Naming\SerializedNameAnnotationStrategy;
use JMS\Serializer\Visitor\Factory\XmlDeserializationVisitorFactory;
use JMS\Serializer\XmlDeserializationVisitor;
Copy link
Contributor

Choose a reason for hiding this comment

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

This unused import statement can be removed

@wing328
Copy link
Member

wing328 commented Mar 29, 2022

It means that after merge we have no Symfony generator for PHP 7.4. I think current Symfony generator should be marked as deprecated and kept as is. While Symfony6 should be new independent generator for php >= 8.0.2 servers. Maybe @wing328 can say how to resolve this situation.

@ybelenko Yes, we can do so (a typical strategy that we've used before)

@wing328 wing328 changed the base branch from 6.0.x to master March 29, 2022 18:21
@wing328
Copy link
Member

wing328 commented Mar 29, 2022

I've just changed the target branch to master, which is the upcoming v6.0.0 release.

@BenjaminHae
Copy link
Contributor Author

I've implemented the suggested changes. It is now also compatible with php 7.4 and symfony 5.
If anything else is needed, please let me know.

@wing328
Copy link
Member

wing328 commented May 23, 2022

Tested with the Travis CI but got the following errors:

68 packages you are using are looking for funding.
Use the `composer fund` command to find out more!
PHP Fatal error:  Declaration of OpenAPI\Server\Tests\Api\PetApiInterfaceTest::tearDown() must be compatible with Symfony\Bundle\FrameworkBundle\Test\WebTestCase::tearDown(): void in /home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/Api/PetApiInterfaceTest.php on line 65
PHP Stack trace:
PHP   1. {main}() /home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/vendor/bin/phpunit:0
PHP   2. include() /home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/vendor/bin/phpunit:120
PHP   3. PHPUnit\TextUI\Command::main($exit = *uninitialized*) /home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/vendor/phpunit/phpunit/phpunit:98
PHP   4. PHPUnit\TextUI\Command->run($argv = [0 => 'vendor/bin/phpunit', 1 => 'Tests'], $exit = TRUE) /home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/vendor/phpunit/phpunit/src/TextUI/Command.php:96
PHP   5. PHPUnit\Runner\BaseTestRunner->getTest($suiteClassFile = '/home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests', $suffixes = [0 => 'Test.php', 1 => '.phpt']) /home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/vendor/phpunit/phpunit/src/TextUI/Command.php:120
PHP   6. PHPUnit\Framework\TestSuite->addTestFiles($fileNames = [0 => '/home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/Api/PetApiInterfaceTest.php', 1 => '/home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/Api/StoreApiInterfaceTest.php', 2 => '/home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/Api/UserApiInterfaceTest.php', 3 => '/home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/Controller/ControllerTest.php', 4 => '/home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/Model/ApiResponseTest.php', 5 => '/home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/Model/CategoryTest.php', 6 => '/home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/Model/InlineObject1Test.php', 7 => '/home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/Model/InlineObjectTest.php', 8 => '/home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/Model/OrderTest.php', 9 => '/home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/Model/PetTest.php', 10 => '/home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/Model/TagTest.php', 11 => '/home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/Model/UserTest.php']) /home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/vendor/phpunit/phpunit/src/Runner/BaseTestRunner.php:98
PHP   7. PHPUnit\Framework\TestSuite->addTestFile($filename = '/home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/Api/PetApiInterfaceTest.php') /home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/vendor/phpunit/phpunit/src/Framework/TestSuite.php:529
PHP   8. PHPUnit\Util\FileLoader::checkAndLoad($filename = '/home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/Api/PetApiInterfaceTest.php') /home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/vendor/phpunit/phpunit/src/Framework/TestSuite.php:401
PHP   9. PHPUnit\Util\FileLoader::load($filename = '/home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/Api/PetApiInterfaceTest.php') /home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/vendor/phpunit/phpunit/src/Util/FileLoader.php:49
PHP  10. include_once() /home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/vendor/phpunit/phpunit/src/Util/FileLoader.php:65
[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.2.1:exec (bundle-test) on project PhpSymfonyTests: Command execution failed. Process exited with an error: 255 (Exit value: 255) -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.2.1:exec (bundle-test) on project PhpSymfonyTests: Command execution failed.

Did the tests pass locally in your machine?

@BenjaminHae
Copy link
Contributor Author

Yes it did.

@wing328
Copy link
Member

wing328 commented May 23, 2022

@BenjaminHae which version of PHP are you using? Travis CI use PHP 8.1 if I'm not mistaken.

@BenjaminHae
Copy link
Contributor Author

@BenjaminHae which version of PHP are you using? Travis CI use PHP 8.1 if I'm not mistaken.
Ah those tests. I never ran these. Fixing them is in progress :) Possibly done within the hour.

@wing328
Copy link
Member

wing328 commented May 24, 2022

Take your time. Let me know when it's ready for review.

Hopefully we can get this in before the v6.0.0 release later this week.

@BenjaminHae
Copy link
Contributor Author

Done :)

@wing328
Copy link
Member

wing328 commented May 25, 2022

Tested it again in Travis CI and I got a lot fo stack trace:

PHP Stack trace:
PHP   1. {main}() /home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/vendor/bin/phpunit:0
PHP   2. include() /home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/vendor/bin/phpunit:120
PHP   3. PHPUnit\TextUI\Command::main($exit = *uninitialized*) /home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/vendor/phpunit/phpunit/phpunit:98
PHP   4. PHPUnit\TextUI\Command->run($argv = [0 => 'vendor/bin/phpunit', 1 => 'Tests'], $exit = TRUE) /home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/vendor/phpunit/phpunit/src/TextUI/Command.php:96
PHP   5. PHPUnit\TextUI\TestRunner->run($suite = class PHPUnit\Framework\TestSuite { protected $backupGlobals = NULL; protected $backupStaticAttributes = NULL; protected $runTestInSeparateProcess = FALSE; protected $name = '/home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests'; protected $groups = ['openapi-generator contributors' => [...], '__phpunit_covers_::iscontenttypeallowed' => [...]]; protected $tests = [0 => class PHPUnit\Framework\TestSuite { ... }, 1 => class PHPUnit\Framework\TestSuite { ... }, 2 => class PHPUnit\Framework\TestSuite { ... }, 3 => class PHPUnit\Framework\TestSuite { ... }, 4 => class PHPUnit\Framework\TestSuite { ... }, 5 => class PHPUnit\Framework\TestSuite { ... }, 6 => class PHPUnit\Framework\TestSuite { ... }, 7 => class PHPUnit\Framework\TestSuite { ... }, 8 => class PHPUnit\Framework\TestSuite { ... }, 9 => class PHPUnit\Framework\TestSuite { ... }]; protected $numTests = 62; protected $testCase = FALSE; protected $foundClasses = [1 => 'PHPUnit\\Framework\\DataProviderTestSuite', 2 => 'PHPUnit\\Util\\Test', 3 => 'PHPUnit\\Util\\Annotation\\Registry', 4 => 'PHPUnit\\Util\\Annotation\\DocBlock', 5 => 'PHPUnit\\Framework\\TestBuilder', 6 => 'PHPUnit\\Framework\\ExecutionOrderDependency', 7 => 'Symfony\\Bundle\\FrameworkBundle\\Test\\WebTestCase', 8 => 'Symfony\\Bundle\\FrameworkBundle\\Test\\KernelTestCase', 9 => 'PHPUnit\\Framework\\TestCase', 10 => 'PHPUnit\\Framework\\Assert']; protected $providedTests = NULL; protected $requiredTests = NULL; private $beStrictAboutChangesToGlobalState = NULL; private $iteratorFilter = NULL; private $declaredClassesPointer = 287; private $warnings = [] }, $arguments = ['extensions' => [], 'listGroups' => FALSE, 'listSuites' => FALSE, 'listTests' => FALSE, 'listTestsXml' => FALSE, 'loader' => NULL, 'useDefaultConfiguration' => TRUE, 'loadedExtensions' => [], 'unavailableExtensions' => [], 'notLoadedExtensions' => [], 'testSuffixes' => [0 => 'Test.php', 1 => '.phpt'], 'configuration' => '/home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/phpunit.xml.dist', 'configurationObject' => class PHPUnit\TextUI\XmlConfiguration\Configuration { private $filename = '/home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/phpunit.xml.dist'; private $validationResult = class PHPUnit\Util\Xml\ValidationResult { ... }; private $extensions = class PHPUnit\TextUI\XmlConfiguration\ExtensionCollection { ... }; private $codeCoverage = class PHPUnit\TextUI\XmlConfiguration\CodeCoverage\CodeCoverage { ... }; private $groups = class PHPUnit\TextUI\XmlConfiguration\Groups { ... }; private $testdoxGroups = class PHPUnit\TextUI\XmlConfiguration\Groups { ... }; private $listeners = class PHPUnit\TextUI\XmlConfiguration\ExtensionCollection { ... }; private $logging = class PHPUnit\TextUI\XmlConfiguration\Logging\Logging { ... }; private $php = class PHPUnit\TextUI\XmlConfiguration\Php { ... }; private $phpunit = class PHPUnit\TextUI\XmlConfiguration\PHPUnit { ... }; private $testSuite = class PHPUnit\TextUI\XmlConfiguration\TestSuiteCollection { ... } }, 'stderr' => FALSE, 'columns' => 80], $warnings = [], $exit = TRUE) /home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/vendor/phpunit/phpunit/src/TextUI/Command.php:143
PHP   6. PHPUnit\Framework\TestSuite->run($result = class PHPUnit\Framework\TestResult { private $passed = []; private $passedTestClasses = []; private $currentTestSuiteFailed = TRUE; private $errors = []; private $failures = []; private $warnings = []; private $notImplemented = []; private $risky = [0 => class PHPUnit\Framework\TestFailure { ... }]; private $skipped = []; private $listeners = [0 => class PHPUnit\Runner\TestListenerAdapter { ... }, 1 => class PHPUnit\TextUI\DefaultResultPrinter { ... }]; private $runTests = 2; private $time = 0.45660316; private $codeCoverage = NULL; private $convertDeprecationsToExceptions = FALSE; private $convertErrorsToExceptions = TRUE; private $convertNoticesToExceptions = TRUE; private $convertWarningsToExceptions = TRUE; private $stop = FALSE; private $stopOnError = FALSE; private $stopOnFailure = FALSE; private $stopOnWarning = FALSE; private $beStrictAboutTestsThatDoNotTestAnything = TRUE; private $beStrictAboutOutputDuringTests = FALSE; private $beStrictAboutTodoAnnotatedTests = FALSE; private $beStrictAboutResourceUsageDuringSmallTests = FALSE; private $enforceTimeLimit = FALSE; private $forceCoversAnnotation = FALSE; private $timeoutForSmallTests = 1; private $timeoutForMediumTests = 10; private $timeoutForLargeTests = 60; private $stopOnRisky = FALSE; private $stopOnIncomplete = FALSE; private $stopOnSkipped = FALSE; private $lastTestFailed = FALSE; private $defaultTimeLimit = 1; private $stopOnDefect = FALSE; private $registerMockObjectsFromTestArgumentsRecursively = FALSE }) /home/travis/build/Op

Ref: https://api.travis-ci.com/v3/job/571312460/log.txt

Did you get something similar locally?

@wing328
Copy link
Member

wing328 commented May 25, 2022

Tested locally with composer2 and the result is ok:

50) OpenAPI\Server\Model\UserTest::testPropertyEmail
This test did not perform any assertions

/home/wing328/Code/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/Model/UserTest.php:114

51) OpenAPI\Server\Model\UserTest::testPropertyPassword
This test did not perform any assertions

/home/wing328/Code/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/Model/UserTest.php:121

52) OpenAPI\Server\Model\UserTest::testPropertyPhone
This test did not perform any assertions

/home/wing328/Code/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/Model/UserTest.php:128

53) OpenAPI\Server\Model\UserTest::testPropertyUserStatus
This test did not perform any assertions

/home/wing328/Code/openapi-generator/samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/Model/UserTest.php:135

OK, but incomplete, skipped, or risky tests!
Tests: 62, Assertions: 9, Risky: 53.
➜  SymfonyBundle-php git:(BenjaminHae-6.0.x-symfony6) ✗ 

@wing328 wing328 merged commit 77fa028 into OpenAPITools:master May 25, 2022
@wing328
Copy link
Member

wing328 commented May 25, 2022

Let's start with what you've so far and we'll improve it over time. 👍

@wing328
Copy link
Member

wing328 commented May 25, 2022

@BenjaminHae I've filed #12455 to make some minor enhancements.

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.

[REQ][PHP][SYMFONY] Upgrade php-symfony code generation to work witH Symfony6
6 participants