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

Make EntityManager @final and its constructor public #9915

Merged
merged 1 commit into from
Jul 28, 2022

Conversation

nicolas-grekas
Copy link
Member

Fix #9896

@nicolas-grekas nicolas-grekas force-pushed the pub-em branch 4 times, most recently from 3827d79 to a070a5c Compare July 19, 2022 06:25
@derrabus
Copy link
Member

Thanks. Can you have a look at the failing tests?

@nicolas-grekas
Copy link
Member Author

Likely not before a week but otherwise yes!
What about removing the last argument of the constructor btw?

@derrabus
Copy link
Member

What about removing the last argument of the constructor btw?

That's probably a good idea.

@nicolas-grekas
Copy link
Member Author

Tests updated, PR should be ready.

@derrabus derrabus requested a review from greg0ire July 27, 2022 18:52
@greg0ire greg0ire added this to the 2.13.0 milestone Jul 28, 2022
@greg0ire
Copy link
Member

There was a cancelled job, I'm running it again, hopefully it will pass. I don't know why it was cancelled.

@greg0ire
Copy link
Member

greg0ire commented Jul 28, 2022

The performance of that job is suspiciously bad: https://github.com/doctrine/orm/runs/7559763390?check_suite_focus=true, even when compared with this other random PR: https://github.com/doctrine/orm/runs/7558748372?check_suite_focus=true
Let me re-run that last one.

@greg0ire
Copy link
Member

It's slower than usual (1m13), but not as slow, let me restart the first one then.

@greg0ire
Copy link
Member

53s 🙂

@greg0ire greg0ire merged commit 2ebe18a into doctrine:2.13.x Jul 28, 2022
@greg0ire
Copy link
Member

Thanks @nicolas-grekas !

@nicolas-grekas nicolas-grekas deleted the pub-em branch July 28, 2022 12:56
@nicolas-grekas
Copy link
Member Author

Thanks for merging \o/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants