-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
The code generated by OpenAPI-generator is a package you can add as a dependency to an existing symfony project. One of the symfony dependencies required in the The normal workflow is (as described in the README created by the generator for
During this workflow you never touch the contents of the generated bundle. |
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 :) |
Ohh, of course. I forgot about that bundle/package driven behaviour in Symfony. Cool, then question is solved.
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); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
@ybelenko Yes, we can do so (a typical strategy that we've used before) |
I've just changed the target branch to master, which is the upcoming v6.0.0 release. |
I've implemented the suggested changes. It is now also compatible with php 7.4 and symfony 5. |
Nice thank you!
Tested with the Travis CI but got the following errors:
Did the tests pass locally in your machine? |
Yes it did. |
@BenjaminHae which version of PHP are you using? Travis CI use PHP 8.1 if I'm not mistaken. |
|
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. |
…ator into 6.0.x-symfony6
Done :) |
Tested it again in Travis CI and I got a lot fo stack trace:
Ref: https://api.travis-ci.com/v3/job/571312460/log.txt Did you get something similar locally? |
Tested locally with composer2 and the result is ok:
|
Let's start with what you've so far and we'll improve it over time. 👍 |
@BenjaminHae I've filed #12455 to make some minor enhancements. |
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 currentphp-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
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.
master
(5.3.0),6.0.x
@jebentier (2017/07), @dkarlovi (2017/07), @mandrean (2017/08), @jfastnacht (2017/09), @ackintosh (2017/09) ❤️, @ybelenko (2018/07), @renepardon (2018/12)