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

[RFC] Tests around reported cases over DDC-2524 #1570

Merged
merged 1 commit into from
Dec 1, 2015
Merged

Conversation

guilhermeblanco
Copy link
Member

Fixes #707

@guilhermeblanco guilhermeblanco self-assigned this Nov 25, 2015
@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-4018

We use Jira to track the state of pull requests and the versions they got
included in.

@guilhermeblanco guilhermeblanco changed the title [RFC] Tests around reported cases over DDC-2524 [WIP] Tests around reported cases over DDC-2524 Nov 25, 2015
@guilhermeblanco
Copy link
Member Author

@mnapoli so far I fixed the testSingle. I'm thinking testMany might not be a problem of CommitOrderCalculator anymore, but rather a Persister issue.

@guilhermeblanco guilhermeblanco changed the title [WIP] Tests around reported cases over DDC-2524 [RFC] Tests around reported cases over DDC-2524 Nov 27, 2015
guilhermeblanco added a commit that referenced this pull request Dec 1, 2015
[RFC] Tests around reported cases over DDC-2524
@guilhermeblanco guilhermeblanco merged commit 9b77ba2 into master Dec 1, 2015
@guilhermeblanco guilhermeblanco deleted the DDC-2524 branch December 1, 2015 05:27
*/
public function addClass($class)
Copy link
Member

Choose a reason for hiding this comment

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

removing public properties is a BC break, which must not be done in minor releases. You broke doctrine/data-fixtures. Please add the methods back

Copy link
Member

Choose a reason for hiding this comment

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

Agree here - the merge has to be reverted and delayed to 3.x :-(

Copy link
Member Author

Choose a reason for hiding this comment

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

This class was never meant to be used by anyone else, not even other Doctrine projects.
data-fixtures should copy the new class and use on its own

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is a more detailed reason why I think this should not be reverted and why data-fixtures usage of the old class here is broken: doctrine/data-fixtures#212 (comment)

Choose a reason for hiding this comment

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

A solution as been decided for this BC break ? We temporarily fix the Doctrine ORM version to 2.5.2 on our project.

Copy link
Member Author

Choose a reason for hiding this comment

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

@fsevestre yes, we'll not revert this commit. Explanation is available at data-fixtures comment I left in previous message.

Choose a reason for hiding this comment

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

@guilhermeblanco Does that mean this fix will land in the next 2.5.x release?

I've got a different issue with commit ordering that this PR solves, so I really need it soon.

Choose a reason for hiding this comment

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

Upon further review, it looks like my issue is likely related to cyclic dependencies, which explains why this fix would solve my issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

It'll only land in 2.6, as this introduces a BC break for data-fixtures project.

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.

7 participants