-
-
Notifications
You must be signed in to change notification settings - Fork 833
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
fix(#2222): Fix properties with default values getting marked as required #2248
fix(#2222): Fix properties with default values getting marked as required #2248
Conversation
11dd3a0
to
3d7b862
Compare
@DjordyKoert tests are failing with PHP 7.4 because of the promoted constructor properties syntax in the two newly added test entities. Do you have an approach in mind how to prevent those failures? I've thought of wrapping the entity classes and the methods within the |
src/ModelDescriber/Annotations/SymfonyConstraintAnnotationReader.php
Outdated
Show resolved
Hide resolved
tests/Functional/Entity/EntityWithPromotedPropertiesWithDefaults81.php
Outdated
Show resolved
Hide resolved
I think it's okay to simply wrap the entity class ( |
@DjordyKoert Thanks for your fast response and the code review/input! I've addressed all the open points and was also able to get the tests running with PHP 7.4. 😊 Unfortunately, the test now fails on PHP 8.0 only and I really can't get my head around why. According to the job log, the
But this isn't the case. I've also manually checked the code beforehand and as far as I'm concerned, it should work. See https://3v4l.org/d0p5L#v8.0.30 with no error output. Am I missing something obvious here? 🤔 😅 |
28a0fa2
to
1d41bf5
Compare
1d41bf5
to
329d232
Compare
Thank you!
I got no idea why your setup wouldn't work but I have moved this this to its own controller instead of building more onto the |
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.
@DominicLuidold could you see if I missed anything by any chance? 😄
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.
Just two nit-picky things. Thanks a lot for your support @DjordyKoert! 🙏
Thank you very much @DominicLuidold! |
This PR fixes the following two bugs:
ReflectionClass::getDefaultProperties()
.1SymfonyConstraintAnnotationReader::processPropertyAnnotations()
method is called before a property's default value is set and additionally doesn't consider a possible set default value.Footnotes
https://bugs.php.net/bug.php?id=81386 ↩