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

Add compatibility with Symfony 6 #710

Merged
merged 1 commit into from
Nov 24, 2021
Merged

Conversation

franmomu
Copy link
Contributor

@franmomu franmomu commented Nov 7, 2021

There are many BC breaks here

  • DoctrineMongoDBExtension: I think they could be acceptable since I think it's quite unusual that someone extends from it.
  • DoctrineMongoDBBundle: The same thing, I think it is unlikely that someone extends it.
  • MongoDBQueryBuilderLoader and DocumentType: I'm not sure about these ones, should we comment on [META] Symfony 6 return type declarations symfony/symfony#43021? or do you think it's safe to add the return types.

Tests will be probably fixed with symfony/symfony#43917

@malarzm
Copy link
Member

malarzm commented Nov 15, 2021

  • DocumentType should be "extended" in a symfony/form-type-way (`::getParent() method IIRC) so this should be safe.
  • MongoDBQueryBuilderLoader how this is solved for ORM?

@franmomu
Copy link
Contributor Author

  • MongoDBQueryBuilderLoader how this is solved for ORM?

It's in Symfony 😬

@jordisala1991
Copy link

@franmomu it does not seem that your PR (symfony/symfony#43917) is getting merged for 5.4 or 6.0 (it is target to 6.1), does that affect this PR in any way?

We only miss this one to add sf 6 to three more Sonata bundles.

@franmomu
Copy link
Contributor Author

@franmomu it does not seem that your PR (symfony/symfony#43917) is getting merged for 5.4 or 6.0 (it is target to 6.1), does that affect this PR in any way?

I've written a comment there, thanks for pointing that out.

In any case, that's when using auto_mapping, we can always set it explicitly, I'll try to change it later today or tomorrow to see if it passes the tests.

@malarzm
Copy link
Member

malarzm commented Nov 18, 2021

@franmomu I think let's add the types everywhere, let's just put it to the UPGRADE file that in an (unlikely ;) ) event somebody did extend our classes, they need to adjust their code. It's a small BC break they'll need to live with :)

@franmomu
Copy link
Contributor Author

@franmomu I think let's add the types everywhere, let's just put it to the UPGRADE file that in an (unlikely ;) ) event somebody did extend our classes, they need to adjust their code. It's a small BC break they'll need to live with :)

👍 I think it's unlikely to happen too, now we need #713 first

@malarzm
Copy link
Member

malarzm commented Nov 22, 2021

#713 merged :)

@franmomu franmomu marked this pull request as ready for review November 23, 2021 07:51
@franmomu franmomu closed this Nov 23, 2021
@franmomu franmomu reopened this Nov 23, 2021
@franmomu franmomu requested a review from malarzm November 23, 2021 07:57
@franmomu
Copy link
Contributor Author

All 🟢 now!

Copy link
Member

@malarzm malarzm left a comment

Choose a reason for hiding this comment

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

🎉

@malarzm malarzm merged commit 71d8382 into doctrine:4.4.x Nov 24, 2021
@malarzm
Copy link
Member

malarzm commented Nov 24, 2021

Thanks @franmomu!

@malarzm malarzm added this to the 4.4.0 milestone Nov 24, 2021
@franmomu franmomu deleted the test_sf6 branch November 25, 2021 08:26
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.

3 participants