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.

Sign up for GitHub

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

Jump to bottom

Fix the handling of circular references in the commit order calculator #7260

Merged
merged 3 commits into from
Sep 23, 2018
Merged

Fix the handling of circular references in the commit order calculator #7260

merged 3 commits into from
Sep 23, 2018

Conversation

stof
Copy link
Member

@stof stof commented Jun 15, 2018 •
edited
Loading

This fixes #7259

On 2.5, both tests are passing. On 2.6.2, the first one is failing while the second one is passing. The only difference between them is the order of persist($fileVersion) and persist($file)

@stof
Copy link
Member Author

stof commented Jun 15, 2018

Note that removing the call to persist $feed after them (which also depends on the Space class involved in the circular reference) or moving them before them also removes the bug.

@stof stof mentioned this pull request Jun 15, 2018
Closed
@stof stof force-pushed the regression_commit_order branch 2 times, most recently from 722005f to 4b72667 Compare September 20, 2018 09:54
@stof
Copy link
Member Author

stof commented Sep 20, 2018

@guilhermeblanco I added another reproducer in a unit test at the commit order calculator level, based on what the UnitOfWork would be configuring in it in the failing functional test.

I kept the 2 functional tests I added previously, as they show high-level usage (and so would be helpful in case of a future refactoring of this part to ensure things keep working).

stof added 2 commits September 20, 2018 12:11
@stof
Add a unit test reproducing the commit order regression
11a7f35
@stof
Fix the computation of commit order for circular dependencies
568c2d3 
When finding a circular dependencies, we must ensure that all dependencies
of a node have been visited before adding it to the sorted list.
@stof stof force-pushed the regression_commit_order branch from 4b72667 to 568c2d3 Compare September 20, 2018 10:14
@stof
Copy link
Member Author

stof commented Sep 20, 2018

@guilhermeblanco the last commit in this PR is fixing the bug, with the idea I suggested in #7259 (comment)

Can you validate whether this approach makes sense ?

@stof stof changed the title Add a test reproducing GH7259 Fix the handling of circular references in the commit order calculator Sep 20, 2018
@guilhermeblanco
Copy link
Member

@stof That totally makes sense. It took me a while to even revive the whole operation/approach, but your solution is appropriate. Very well done! Thanks!!!

@guilhermeblanco guilhermeblanco self-requested a review September 21, 2018 02:16
@guilhermeblanco
Copy link
Member

guilhermeblanco commented Sep 21, 2018 •
edited
Loading

I'm allowing someone else merge as I got very detached from 2.X land and don't have full context on merge process now between 2.5, 2.6 and 2.7.

@Majkl578 Majkl578 added this to the 2.6.3 milestone Sep 21, 2018
@Majkl578 Majkl578 self-assigned this Sep 21, 2018
@stof
Copy link
Member Author

stof commented Sep 21, 2018

@guilhermeblanco 2.5 is not maintained anymore (except maybe for security patches, but I'm not even sure). regarding 2.6 vs 2.7, I don't know what is the merge process for maintainers (but this bugfix should definitely go in the next 2.6 patch release as it fixes a regression)

@Majkl578 Majkl578 merged commit fd2baf6 into doctrine:2.6 Sep 23, 2018
@Majkl578 Majkl578 mentioned this pull request Sep 23, 2018
Merged
@Majkl578
Copy link
Contributor

Thanks @stof. 🚢 Will appear in 2.6.3.

@stof stof deleted the regression_commit_order branch September 24, 2018 13:04
@bendavies bendavies mentioned this pull request Oct 8, 2018
Closed
ixarlie added a commit to surexdirect/doctrine2 that referenced this pull request Jul 10, 2019
@ixarlie
Merge tag 'v2.6.3' into comparison_strategy_v2.6.3
786497f 
v2.6.3

[![Build Status](https://travis-ci.org/doctrine/doctrine2.svg?branch=v2.6.3)](https://travis-ci.org/doctrine/doctrine2)

This release provides fixes for many things, specially:

- Regression in commit order calculation
- BC-break in `EntityManager#find()` using optimistic lock outside of
  transaction
- PHP 7.3 compatibility issues

--------------------------------------------

- Total issues resolved: **8**
- Total pull requests resolved: **26**
- Total contributors: **26**

Documentation
-------------

 - [7472: fix incorrect phpdoc typehint](doctrine#7472) thanks to @seferov
 - [7465: Fixes tiny typo in the 'Working with DateTime instances' documentation](doctrine#7465) thanks to @unguul
 - [7444: Fixed URLs of doctrine-mapping.xsd in docs](doctrine#7444) thanks to @Naitsirch
 - [7441: $hydrationMode throughout can be a string as well as int (for custom modes)](doctrine#7441) thanks to @asgrim
 - [7435: Fix a typo on Documentation](doctrine#7435) thanks to @oguzdumanoglu
 - [7434: Removed FAQ paragraph stating public variables are disallowed](doctrine#7434) thanks to @Naitsirch and @flaushi
 - [7423: Update association-mapping.rst](doctrine#7423) thanks to @ThomasLandauer
 - [7421: JIRA to Github issues on Limitations and Known Issues](doctrine#7421) thanks to @seferov
 - [7412: Some formatting improvements](doctrine#7412) thanks to @ThomasLandauer
 - [7411: Autoload error when following the Getting Started Guide](doctrine#7411) thanks to @ThomasLandauer
 - [7401: &doctrine#91;docs&doctrine#93; Fix docblock in `inheritance-mapping.rst`](doctrine#7401) thanks to @bobdenotter
 - [7397: Update getting-started.rst](doctrine#7397) thanks to @eibt
 - [7394: Class 'Doctrine\Common\Persistence\Mapping\Driver\AnnotationDriver' not found](doctrine#7394) thanks to @ekosynth
 - [7378: Typo fix](doctrine#7378) thanks to @BenMorel
 - [7377: Fix query andX doctype](doctrine#7377) thanks to @sserbin
 - [7374: Deprecation message in documentation for YAML](doctrine#7374) thanks to @SenseException and @iltar
 - [7360: Document getPartialReference() properly](doctrine#7360) thanks to @lcobucci

Bug
---

 - [7471: Fix parameter value processing for objects with unloaded metadata](doctrine#7471) thanks to @alcaeus
 - [7367: Fix for BC break in 2.6.2 when calling EM::find() with LockMode::OPTIMISTIC outside of a TX](doctrine#7367) thanks to @timdev
 - [7328: Handle removed parameters by tree walker in Paginator](doctrine#7328) thanks to @plfort
 - [7325: Make code php 7.3 lint-compatible](doctrine#7325) thanks to @paxal
 - [7317: &doctrine#91;XML&doctrine#93; Fix default value of many-to-many order-by to ASC](doctrine#7317) thanks to @alexdenvir
 - [7260: Fix the handling of circular references in the commit order calculator](doctrine#7260) thanks to @stof
 - [6830: fix applying column options on foreign key columns](doctrine#6830) thanks to @Tobion

Improvement
-----------

 - [7428: CI: Test against PHP 7.3](doctrine#7428) thanks to @Majkl578
 - [7363: Fix compatibility with phan](doctrine#7363) thanks to @philippe-unitiz
 - [7345: Correct DOMDocument constructor in test](doctrine#7345) thanks to @guilliamxavier
 - [7307: Fix remaining usages of deprecated ClassLoader and Inflector from doctrine/common](doctrine#7307) thanks to @Majkl578 and @simonwelsh
@mpdude mpdude mentioned this pull request Mar 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guilhermeblanco guilhermeblanco guilhermeblanco approved these changes

Assignees

@Majkl578 Majkl578

Labels
Bug
Projects
None yet
Milestone
2.6.3
Development

Successfully merging this pull request may close these issues.
3 participants
@stof @guilhermeblanco @Majkl578