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 wiring of PHP metadata drivers on doctrine/orm 3.x #1832

Merged
merged 6 commits into from
Nov 6, 2024

Conversation

MatTheCat
Copy link
Contributor

@MatTheCat MatTheCat commented Nov 4, 2024

Follows symfony/symfony#33319, takes over #1755 and fixes symfony/symfony#58738.

Some metadata drivers were removed in doctrine/orm 3 and must then be used from doctrine/persistence.

Since the parameters used to infer their class name are deprecated, this PR rewrites getMetadataDriverClass so that it selects the right one.

Note that some class parameters are still used in the DoctrineExtension but I considered them out of this PR’s scope.

@ostrolucky
Copy link
Member

Looks like tests will need fixing. And for SA issues you can suppress them.

@MatTheCat
Copy link
Contributor Author

Hm should I split tests into AnnotationDriver-exists-or-not? Or sprinkle them with class_exists? 🤔

@ostrolucky
Copy link
Member

My preference would be to upgrade current tests to use attribute instead of annotation and skip them for orm < 3.x

@stof
Copy link
Member

stof commented Nov 4, 2024

We don't need to skip them for ORM 2.x AFAIK. Attributes are supported there as well (on PHP 8+)

@MatTheCat
Copy link
Contributor Author

Do I keep the DoctrineExtensionTest::testAnnotationsBundleMappingDetection?

@ostrolucky
Copy link
Member

you can remove it

@MatTheCat MatTheCat force-pushed the 2.13.x branch 3 times, most recently from b814469 to 8040928 Compare November 4, 2024 20:36
@MatTheCat
Copy link
Contributor Author

Okay, done. Not sure why some tests are still failing though 🤔

@ostrolucky
Copy link
Member

@MatTheCat
Copy link
Contributor Author

Waat so the Doctrine Bridge does need to be updated 😅

@ostrolucky
Copy link
Member

I don't know, just saying that's the cause why tests fail. Proper course of action would be to find out why was that code branch added and find if same reasoning still applies nowadays. Then we know if we need to preserve the behavior somehow, or adjust tests

@MatTheCat
Copy link
Contributor Author

See symfony/symfony#53681; seems that we cannot be compatible with Symfony < 6.4 by returning XML and YAML drivers’ class name instead of the parameter.

@MatTheCat
Copy link
Contributor Author

MatTheCat commented Nov 5, 2024

Maybe this PR should focus to simply returning the correct PHP driver’s class name? That would fix symfony/symfony#58738, hopefully without any side effect.

@ostrolucky
Copy link
Member

I believe we just need to backport symfony/symfony#53681 to 5.4 branch. It's either that, or we cut off symfony/doctrine-bridge 5.4.x support here

@MatTheCat
Copy link
Contributor Author

Okay opened symfony/symfony#58772

@MatTheCat
Copy link
Contributor Author

MatTheCat commented Nov 6, 2024

Woops this got closed with symfony/symfony#58772 and I cannot reopen it 😓

Can someone do it for me or should I open another PR?

EDIT: thanks @nicolas-grekas 🙏

@nicolas-grekas nicolas-grekas reopened this Nov 6, 2024
@ostrolucky
Copy link
Member

ostrolucky commented Nov 6, 2024

Last thing needed here I think is to adjust symfony/doctrine-bridge requirement to ensure we don't install < 5.4.46 version

@MatTheCat
Copy link
Contributor Author

Shouldn’t the ^6.0.7 constraint be bumped to ^6.4.3 and ^7.0 to ^7.0.3 so that we’re sure we got the fix?

@ostrolucky
Copy link
Member

indeed

@MatTheCat
Copy link
Contributor Author

CI is green 🎉

@ostrolucky ostrolucky changed the title Do not rely on parameters to get metadata drivers class Fix wiring of PHP metadata drivers on doctrine/orm 3.x Nov 6, 2024
@ostrolucky ostrolucky added the Bug label Nov 6, 2024
@ostrolucky ostrolucky added this to the 2.13.1 milestone Nov 6, 2024
@ostrolucky ostrolucky merged commit eda148b into doctrine:2.13.x Nov 6, 2024
16 checks passed
@MatTheCat MatTheCat deleted the 2.13.x branch November 6, 2024 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

doctrine type 'php' and 'staticphp' use wrong doctrine class name
4 participants