Skip to content

[Validator] Fix regression with class metadatada on parent classes #50788

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 20, 2023
Merged

[Validator] Fix regression with class metadatada on parent classes #50788

merged 1 commit into from
Jul 20, 2023

Conversation

rmikalkenas
Copy link
Contributor

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #50780
License MIT

@carsonbot carsonbot added this to the 5.4 milestone Jun 27, 2023
@rmikalkenas rmikalkenas marked this pull request as draft June 27, 2023 06:57
@rmikalkenas rmikalkenas force-pushed the ticket_50780 branch 3 times, most recently from 05b8978 to 60bd664 Compare June 27, 2023 13:15
@rmikalkenas rmikalkenas marked this pull request as ready for review June 27, 2023 13:53
@rmikalkenas
Copy link
Contributor Author

Unit Tests / Unit Tests (8.2, high-deps) - looks like failing TimezoneValidatorTest::testValidTimezones with data set #9 ('America/Montreal') is unrelated with the changes.

Unit Tests / Unit Tests (8.2, low-deps) - DoctrineLoaderTest::testLoadClassMetadata failing, because doctrineMetadata->fieldMappings does not contain DoctrineLoaderParentEntity::publicParentMaxLength field. Could it be due to some doctrine bug in lower versions..? Need some help with this one

cc @nicolas-grekas

nicolas-grekas
nicolas-grekas reviewed Jul 7, 2023
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

For deps=high, the failure is a false positive.
For deps=low, it's something we need to figure out.

I force-pushed a minor change, see https://github.com/symfony/symfony/compare/60bd66422334099482c110206619eb54cd187c0a..2ba0f57c655ab9921cbaeeb2c60bdb7de781f23c

You can reproduce the issue locally by requiring orm ~2.15.0:

COMPOSER_ROOT_VERSION=5.4.x-dev composer require --dev doctrine/orm:~2.15.0
./phpunit src/Symfony/Bridge/Doctrine/ --filter testLoadClassMetadata

Does this help?

This was referenced Jul 20, 2023
Closed
Closed
Closed
nicolas-grekas
nicolas-grekas approved these changes Jul 20, 2023
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I figured out the proper fix on DoctrineLoader.

@nicolas-grekas
Copy link
Member

Thank you @rmikalkenas.

@nicolas-grekas nicolas-grekas merged commit 5f08512 into symfony:5.4 Jul 20, 2023
This was referenced Jul 29, 2023
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Bug Status: Reviewed Validator
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
3 participants
@rmikalkenas @nicolas-grekas @carsonbot