Skip to content

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

Merged
guilhermeblanco merged 1 commit intomasterfrom
DDC-2524
Dec 1, 2015
Merged

[RFC] Tests around reported cases over DDC-2524#1570
guilhermeblanco merged 1 commit intomasterfrom
DDC-2524

Conversation

@guilhermeblanco
Copy link
Copy Markdown
Member

Fixes #707

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

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
Copy Markdown
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
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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)

Copy link
Copy Markdown

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
Copy Markdown
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.

Copy link
Copy Markdown

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.

Copy link
Copy Markdown

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
Copy Markdown
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