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

Broken commit order in 2.6 #7259

Closed
stof opened this issue Jun 15, 2018 · 5 comments
Closed

Broken commit order in 2.6 #7259

stof opened this issue Jun 15, 2018 · 5 comments
Assignees
Milestone

Comments

@stof
Copy link
Member

stof commented Jun 15, 2018

BC Break Report

Q A
BC Break yes
Version 2.6

Summary

Commit order is wrong in some cases, leading to broken inserts (inserting without the id for the foreign key). this is dependant on the order of persist() calls.

Previous behavior

Flushing was working fine

Current behavior

Flush is failing by trying to insert NULL in a non-nullable foreign key (probably planning to update the field later).

How to reproduce

See #7260 which provides some tests reproducing the issue.

@stof
Copy link
Member Author

stof commented Jun 15, 2018

@guilhermeblanco as you rewrote the CommitOrderCalculator in 2.6 (in #1570), you may want to look at this issue

@pestaa
Copy link

pestaa commented Jul 11, 2018

When both the vertex and the adjacent vertex are in progress, the adjacent vertex might get added to the sorted node list, even though it could have other non-visited dependencies.

It gives correct results if you first sort vertex dependencies by descending weight:

    private function visit($vertex)
    {
        $vertex->state = self::IN_PROGRESS;

        $dependencyList = $vertex->dependencyList;
        usort($dependencyList, function ($a, $b) {
            return $b->weight <=> $a->weight;
        });

        foreach ($dependencyList as $edge) {
            $adjacentVertex = $this->nodeList[$edge->to];

@Majkl578 Majkl578 added this to the 2.6.x milestone Jul 11, 2018
@Ocramius Ocramius modified the milestones: 2.6.x, 2.6.3 Jul 19, 2018
@stof
Copy link
Member Author

stof commented Aug 21, 2018

@guilhermeblanco have you had any chance to take a look at it ?

@stof
Copy link
Member Author

stof commented Sep 20, 2018

@guilhermeblanco I think that when reaching an in-progress node, instead of adding it immediately in the list, we might want to trigger the processing of its own dependencies which are not yet in progress (in case we reached the cycle through the first dependency, and so second dependency was not processed yet). Does it look sensible to you ?

@Majkl578
Copy link
Contributor

Fixed in #7260.

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

No branches or pull requests

5 participants