Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Dynamically fix compatibility with doctrine/data-fixtures v2#58865

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

greg0ire
Copy link
Contributor

@greg0iregreg0ire commentedNov 13, 2024•
edited
Loading

QA
Branch?5.4
Bug fix?yes
New feature?no
Deprecations?no
Issuessee explanation below
LicenseMIT

While working onallowing v2 of doctrine/data-fixtures in the bundle, I stumbled upon an issue that only affects some versions of Symfony that still have aContainerAwareLoader class.

The signature ofContainerAwareLoader::addFixture() is not compatible with the v2 signature of theLoader interface fromdoctrine/data-fixtures, as per this fatal error:

Declaration of Symfony\Bridge\Doctrine\DataFixtures\ContainerAwareLoader::addFixture(Doctrine\Common\DataFixtures\FixtureInterface $fixture)             must be compatible with Doctrine\Common\DataFixtures\Loader::addFixture(Doctrine\Common\DataFixtures\FixtureInterface $fixture): void

Classes that extend ContainerAwareLoader have to also extend Loader, meaning this is no breaking change because it can be argued that the incompatibility of the extending class would be with the Loader interface.

Closes#58861,Closes#58863

@greg0ire
Copy link
ContributorAuthor

How should I deal with the Psalm failures?

@derrabus
Copy link
Member

How should I deal with the Psalm failures?

Ignore them.

greg0ire reacted with thumbs up emoji

@greg0ire
Copy link
ContributorAuthor

Ignore them.

I'll assume you mean that literally because I couldn't find a baseline orpsalm-suppress annotations to ignore them programmatically 😆

@derrabus
Copy link
Member

Ignore them.

I'll assume you mean that literally

Yes. 😎

@chalasr
Copy link
Member

Not sure if the failing test is related?

@greg0ire
Copy link
ContributorAuthor

The Windows test? It's about http-foundation 🤔
Also, it fails on other PRs.

If you're talking about Psalm, the failure is definitely related, but I was told above to ignore it.

@chalasr
Copy link
Member

Sorry, I mean this one on the high-deps build:https://github.com/symfony/symfony/actions/runs/11836984636/job/32983037386?pr=58865.
That test seems to pass on current 5.4, can you please have a look?

@greg0ire
Copy link
ContributorAuthor

greg0ire commentedNov 15, 2024•
edited
Loading

Oh right, that one does look like it could be related. I'll take a look.

I introduced the failing assert indoctrine/persistence 3.3.3:doctrine/persistence#375
Some passing builds allow a version higher than that though, that means there must be another explanation.

The object that does not pass the instanceof check is instanciated by a container that itself is created in a very fishy way. This line in particular is interesting:

$container->getDefinition('foo')->setLazy(true)->addTag('proxy', ['interface' => ObjectManager::class]);

I suspect the feature it leverages is not implemented when using the deps of the 8.2 job. I don't quite understand why it would break only now though. Maybe allowingdoctrine/data-fixtures 2 resulted in slightly different deps?

@greg0ire
Copy link
ContributorAuthor

@MatTheCat maybe you can help with this?

@greg0ire
Copy link
ContributorAuthor

Just noticed the DI tag present in the line I mentioned is missing on 5.4 and was introduced in6a4d229 , hence my last commit, which sadly doesn't seem very successful.

@greg0iregreg0ireforce-pushed thefix-data-fixtures-v2-compat branch from10ff8aa to928bf70CompareNovember 15, 2024 15:55
@greg0ire
Copy link
ContributorAuthor

Ah this might be what I need to backport:#57489

@MatTheCat
Copy link
Contributor

Err I don’t think I can help 😅 ; I recently committed to the DoctrineBridge but it doesn’t look like it was related.

@greg0ire
Copy link
ContributorAuthor

Yes, sorry!

@greg0iregreg0ireforce-pushed thefix-data-fixtures-v2-compat branch from928bf70 to0abc40dCompareNovember 15, 2024 16:50
@greg0iregreg0ireforce-pushed thefix-data-fixtures-v2-compat branch fromf23ae10 toe20459aCompareNovember 15, 2024 16:55
@greg0ire
Copy link
ContributorAuthor

This only happens inhigh-deps mode, which apparently checks out 4.4, that's quite peculiar. At this point I don't really understand what's going on.@xabbuh maybe you would?

@xabbuh
Copy link
Member

@greg0ire you can ignore the high deps failures

greg0ire and chalasr reacted with thumbs up emoji

Classes that extend ContainerAwareLoader have to also extend Loader, meaningthis is no breaking change because it can be argued that the incompatibility ofthe extending class would be with the Loader interface.
@nicolas-grekas
Copy link
Member

Thank you@greg0ire.

greg0ire and Chris53897 reacted with heart emoji

@nicolas-grekasnicolas-grekas merged commitc098762 intosymfony:5.4Nov 20, 2024
9 of 12 checks passed
@greg0iregreg0ire deleted the fix-data-fixtures-v2-compat branchNovember 20, 2024 13:09
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@derrabusderrabusderrabus approved these changes

@xabbuhxabbuhxabbuh approved these changes

@chalasrchalasrchalasr approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.

7 participants
@greg0ire@derrabus@chalasr@MatTheCat@xabbuh@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp