[DDC-3160] Alternate fix for DDC-2996 bug #1074
Conversation
|
To be more specific, I really only tested the Tree features of the DoctrineExtensions project with this fix. |
|
Just tried the fix with the full test suite of DoctrineExtensions, and it seemed to report OK. |
|
👍 and this should be backported to 2.4 (and released as 2.4.4) as 2.4.3 is currently broken, while required to use latest PHP versions |
|
Nice work! +1 for backporting to 2.4. |
|
Would be awesome to make DoctrineExtensions::Tree work again! ⭐ ⛵ 🍎 💾 |
|
👍 |
|
👍, without this we can't update Sylius to use latest stable Doctrine -.- |
|
I'd still ask for a test case to avoid regressions. |
|
👍 |
1 similar comment
|
👍 |
|
👎 until we see a test... =\ |
|
Another user already put together a test for DDC-3160. It is currently a pull request: Should I make another? I looked over the test and it appears to me to be good. Before this fix, the test failed and now it passes. There should already be a test for DDC-2996 that is still passing with this fix: |
|
you should rebase your branch on top of his commit, so that the test gets included in your PR (but preserving authorship) |
|
I just merged the test created by @flack into this pull request. |
| } | ||
| } | ||
|
|
||
| class OnFlushListener |
There was a problem hiding this comment.
Looks like there is already an OnFlushListener in DDC2790Test.php causing an error.
There was a problem hiding this comment.
Always prefix with ticket number... DDC3160OnFlushListener
A fix for DDC-2996 was implemented that broke quite a few extensions. This commit is an attempt to fix the DDC-2996 bug without the adverse side effects seen in DDC-3160. Basically, if changes are detected that would cause a changeset to be made, but the entity is awaiting insertion, the code will not save the changeset nor flag the entity as awaiting updating in the Unit of Work. Some styling tweaks based on Pull Request guidelines.
|
Ok, I think I've got it situated better now (just starting to work with Git and not too knowledgeable of it yet). Based on the feedback from @stof, I've got this branch rebased on the commit by @flack for the DDC-3160 test. I have also fixed up a few issues with that test. The DDC-2996 and the DDC-3160 tests are passing (tried them individually with PHPUnit) along with all the other tests. It also appears that the Travis CI build is passing as well. |
|
This looks good to me. I'm merging into master, so please check it in your local applications with the latest ORM. If the fix actually works for Gedmo, then we'll backport to |
[DDC-3160] Alternate fix for DDC-2996 bug
|
Please report any feedback on this fix at http://www.doctrine-project.org/jira/browse/DDC-3208 |
The fix implemented for DDC-2996 seems to have broken quite a bit of code outside of Doctrine (for instance, the popular DoctrineExtensions project).
I'm not too well versed in Doctrine internals, but looking at the fix, I found it odd that an entity would live in both the insertion and update tracking in the unit of work.
This implementation should fix the case described in DDC-2996, while not creating any side effects with objects that are scheduled to be inserted.
I've ran this test with the Doctrine test suite and have also tested this change with the DoctrineExtensions project; unless I'm doing something wrong, it seems to have good results.
Again, I'm not too well versed in the inner workings of Doctrine, but I did a little research, poked around some code, and came up with this.