Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

This should resolve changes made in the latest release of doctrine2 #212

Conversation

macnibblet
Copy link
Contributor

@stof
Copy link
Member

stof commented Dec 1, 2015

I prefer fixing the BC break in Doctrine ORM instead, as this should not happen in the first place (it breaks the semver policy): doctrine/orm#1570 (comment)

@guilhermeblanco
Copy link
Member

WTH... This class was never meant to be used by anyone... not even other Doctrine projects.
Oh private classes... how much I miss you.

@guilhermeblanco
Copy link
Member

My suggestion is to copy the new class and make it part of data-fixtures project.
New class performs better than old one and handles cyclic dependencies

@guilhermeblanco
Copy link
Member

Let me explain why data-fixtures should not use ORM class at all and might actually copy the class from 2.0 branch https://github.com/doctrine/data-fixtures/blob/2.0/lib/Doctrine/Fixture/Sorter/TopologicalSorter.php

data-fixtures cannot accept cyclic dependencies. Old ORM implementation was loose and accepting cycles, and the implementation was relying which class got added first. Consuming the same implementation, a worse issue exist in data-fixtures project, because of FK constraints check could not be inferred depending of the filesystem order for loading the classes (related to order of add class on ORM), potentially creating an unusable scenario.

Also, data-fixtures cannot be loose on cycles and must notify the user he is making a mistake by breaking the execution. This is what 2.0 implementation does and 1.X version should too. Accepting loose cycles lead to unpredictable execution logic, which may enter in either deadlocks or FK constraint errors.

-1 on reverting on ORM and +1 on bringing the new, reliable execution checker to this version and release as another minor.

@petrjirasek
Copy link

Hello, I would like to ask you if there is any progress with this? I have updated my dependencies on doctrine and doctrine fixtures and now there is the error during the purging.

Thanks for answer

@jurchiks
Copy link

Hey guys, I'm updating my project from Symfony 2.7 to Symfony 3 and all is going well with the only exception being this error being thrown during doctrine:fixtures:load: Call to undefined method Doctrine\ORM\Internal\CommitOrderCalculator::addClass(). This library is marked as Symfony 3-compatible... But apparently it is not?
Can you fix this or should I look for other fixture bundles?

I see people have been reporting this issue for over two months now, so why hasn't this been fixed yet?

@Ocramius
Copy link
Member

@jurchiks that is a problem that affects master only. Also, it is to be fixed in fixtures (fixtures should not use a class from the Internal namespace

@jurchiks
Copy link

You mean master of this project or DoctrineFixturesBundle? Because I'm using version 2.3.0 of DoctrineFixturesBundle and it uses version 1.0 of this project: https://github.com/doctrine/DoctrineFixturesBundle/blob/2.3.0/composer.json#L26.
There is no master anywhere, but I'm getting the same error.

@jurchiks
Copy link

Never mind, composer.lock contained a different version than composer.json, all because one dev-dependency had no stable versions and so "minimum-stability: dev" was required in composer.json...

@Ocramius
Copy link
Member

We have ORM 2.6 blocked anyway until this is resolved.

The correct solution is the one suggested by @guilhermeblanco: duplicate the code into this package (and test it)

@geofrwa
Copy link

geofrwa commented Mar 21, 2016

Thanks, that does the trick (should have read this better -__-' not to delete my question, that was about how to fix the bug without having actually tested the commit fix)

@stevro
Copy link

stevro commented Apr 21, 2016

When could this fix be merged? Thank you.

@guilhermeblanco
Copy link
Member

Closing this one in favor of #222

@guilhermeblanco guilhermeblanco self-assigned this May 5, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants