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

Remove copy() method from EntityManager #6794

Merged
merged 1 commit into from
Dec 8, 2017
Merged

Remove copy() method from EntityManager #6794

merged 1 commit into from
Dec 8, 2017

Conversation

SenseException
Copy link
Member

@SenseException SenseException commented Oct 25, 2017

The method EntityManager::copy() is a pretty old addition to Doctrine and never got any functionality, which is why I created this PR to remove this method from the EntityManager.

Because I had to remove this method from EntityManagerInterface too, I consider this a BC break. Even though the original EntityManager never got any functionality for copy, a user may have created a custom entity manager for a project, that uses a custom implementation of copy.

Questions:

  • Do we need tests for the removal of a method?
  • Do we need a deprecation PR as an in between step?

@alcaeus
Copy link
Member

alcaeus commented Oct 25, 2017

FYI, this PR should target develop as it's the development branch for 3.0.

Do we need tests for the removal of a method?

IMO, no.

Do we need a deprecation PR as an in between step?

Yes. Deprecation needs to happen in master.

@SenseException
Copy link
Member Author

@alcaeus Should I close this PR and recreate it with a branch from develop for the copy-removal?

@alcaeus
Copy link
Member

alcaeus commented Oct 25, 2017

You can rebase this branch on top of develop and change the base of the pull request:
image

@SenseException SenseException changed the base branch from master to develop October 25, 2017 21:21
@lcobucci
Copy link
Member

@guilhermeblanco was rebasing develop to sync with the latest changes in master, so I'd recommend to ensure that he's done with it before of merging this PR

@SenseException
Copy link
Member Author

The deprecation PR to master will follow.

@SenseException
Copy link
Member Author

The deprecation PR is done in #6803.

@Majkl578
Copy link
Contributor

Majkl578 commented Dec 4, 2017

This will need a rebase now.

Copy link
Contributor

@Majkl578 Majkl578 left a comment

Choose a reason for hiding this comment

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

LGTM.
Travis build is missing for unknown reason but it's tests are currently not working on develop branch anyway.

@Majkl578 Majkl578 added this to the 3.0 milestone Dec 8, 2017
@Majkl578 Majkl578 merged commit 73ba8e0 into doctrine:develop Dec 8, 2017
@Majkl578
Copy link
Contributor

Majkl578 commented Dec 8, 2017

@SenseException Thanks. Let's handle deprecation for 2.x in #6803 now.

@greg0ire greg0ire removed this from the 3.0.0 milestone Jun 27, 2021
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.

5 participants