NOTIFY change tracking policy fixes #1257
Conversation
rbrtbn
changed the title
|
@banrobert thank you for PR! Before I get to review could you please take a look at failing test on Travis? |
|
Updated the test case that failed previously, it should work now. The unit tests now passed on Travis CI, but the Scrutinizer has timed out. Maybe a retry would help? |
| unset($this->documentChangeSets[$oid]); | ||
| unset($this->scheduledForDirtyCheck[$class->name][$oid]); | ||
| } elseif (is_array($document)) { | ||
| foreach ($document as $object) { |
There was a problem hiding this comment.
This will not be enough as inserted documents should have their change sets cleared as well (and embedded documents that were committed etc), see #1149
There was a problem hiding this comment.
Having said that I think the PR will be better without this at all and instead we should work on #1149 for 1.x
There was a problem hiding this comment.
You might be right. Until a decision is made on #1149 it's not certain that this fix is what needed here and as you said it's not even complete. I'll remove this from the PR.
…ror_get_last() for catching an E_NOTICE error
|
Removed the commits, never rebased before, didn't know this was possible, cool feature. And regarding #1149 i would also say that implementing the single document flush differently than the ORM but making it clearer by not flushing anything else that is not related to the document in question is the way to go. Hope the team agrees! |
NOTIFY change tracking policy fixes
|
Merged, thank you @banrobert! |
This PR fixes some issues with documents using the NOTIFY change tracking policy, namely:
Undefined indexnotice as reported in [UOW] Only get change set from documentChangeSets if it exists. #740. Fixed the error and created a test case for it.UnitOfWorkTest::testChangeTrackingNotify) used a document without the required@ODM\ChangeTrackingPolicy("NOTIFY")annotation as its test subject, so the test didn't test what it supposed to test. I added the missing annotation to this dummy document.