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
nicolas-grekas merged 1 commit into from
Jul 20, 2023
Merged

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

nicolas-grekas merged 1 commit into from
Jul 20, 2023

Conversation

@rmikalkenas
Copy link
Contributor

@rmikalkenas rmikalkenas commented Jun 27, 2023

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

rmikalkenas commented Jun 27, 2023

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

nicolas-grekas commented Jul 20, 2023

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