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

fix(#2222): Fix properties with default values getting marked as required #2248

Conversation

DominicLuidold
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #2222

This PR fixes the following two bugs:

  • Promoted properties with a value initialized by the constructor are not considered to have a default value and are therefore not returned by ReflectionClass::getDefaultProperties().1
  • The SymfonyConstraintAnnotationReader::processPropertyAnnotations() method is called before a property's default value is set and additionally doesn't consider a possible set default value.

Footnotes

  1. https://bugs.php.net/bug.php?id=81386

@DominicLuidold DominicLuidold changed the title fix(#2222): Fix default value properties getting marked as required fix(#2222): Fix properties with default values getting marked as required Mar 29, 2024
@DominicLuidold DominicLuidold force-pushed the 2222-fix-properties-with-default-values branch from 11dd3a0 to 3d7b862 Compare March 29, 2024 21:45
@DominicLuidold
Copy link
Contributor Author

DominicLuidold commented Mar 29, 2024

@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 ApiControllers with if (PHP_VERSION_ID >= 80000) { but I'm not sure if that's the correct approach here.. 🤔

@DjordyKoert
Copy link
Collaborator

@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 ApiControllers with if (PHP_VERSION_ID >= 80000) { but I'm not sure if that's the correct approach here.. 🤔

I think it's okay to simply wrap the entity class (EntityWithPromotedPropertiesWithDefaults80) with an if-else statement and create an empty class for PHP 7.4, the class should and cannot be tested on 7.4 anyway

@DominicLuidold
Copy link
Contributor Author

DominicLuidold commented Mar 30, 2024

@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 EntityWithPromotedPropertiesWithDefaults80 is declared more than once:

PHP Fatal error:  Cannot declare class EntityWithPromotedPropertiesWithDefaults80, because the name is already in use in /home/runner/work/NelmioApiDocBundle/NelmioApiDocBundle/tests/Functional/Entity/EntityWithPromotedPropertiesWithDefaults80.php(17) : eval()'d code on line 4

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? 🤔 😅

@DjordyKoert DjordyKoert force-pushed the 2222-fix-properties-with-default-values branch from 28a0fa2 to 1d41bf5 Compare March 30, 2024 14:13
@DjordyKoert DjordyKoert force-pushed the 2222-fix-properties-with-default-values branch from 1d41bf5 to 329d232 Compare March 30, 2024 14:17
@DjordyKoert
Copy link
Collaborator

@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. 😊

Thank you!

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 EntityWithPromotedPropertiesWithDefaults80 is declared more than once:

PHP Fatal error:  Cannot declare class EntityWithPromotedPropertiesWithDefaults80, because the name is already in use in /home/runner/work/NelmioApiDocBundle/NelmioApiDocBundle/tests/Functional/Entity/EntityWithPromotedPropertiesWithDefaults80.php(17) : eval()'d code on line 4

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 3v4l.org/d0p5L#v8.0.30 with no error output. Am I missing something obvious here? 🤔 😅

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 ApiController..

Copy link
Collaborator

@DjordyKoert DjordyKoert left a 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? 😄

Copy link
Contributor Author

@DominicLuidold DominicLuidold left a 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! 🙏

@DjordyKoert DjordyKoert merged commit 7f46161 into nelmio:master Mar 30, 2024
11 checks passed
@DjordyKoert
Copy link
Collaborator

Thank you very much @DominicLuidold!

@DominicLuidold DominicLuidold deleted the 2222-fix-properties-with-default-values branch March 30, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Properties with default values still get marked as required
2 participants