Make sure locale doesn't change after flush is called #159
Conversation
| // reset locale to the original locale | ||
| if (null !== $locale) { | ||
| $meta = $dm->getMetadataFactory()->getMetadataFor(get_class($document)); | ||
| $dm->findTranslation($meta->getName(), $meta->getIdentifierValue($document), $locale); |
There was a problem hiding this comment.
you should use $uow->getDocumentId($document) instead. reading the $meta->getIdentifierValue code, it seems that the id could be not mapped on the document.
There was a problem hiding this comment.
Thanks, didn't know such a method existed on the uow.
There was a problem hiding this comment.
|
+1 nice work @wouterj -- whats the problem with testing? too much to setup? |
|
Added a test & fixed the comments. @dantleech couldn't think of a way to bind different locales and then make sure the locale is not the last bound locale before calling flush. Now I realized it's just as simple as calling findTranslation before flushing... |
| if (null !== $locale) { | ||
| $dm->findTranslation(get_class($document), $uow->getDocumentId($document), $locale); | ||
|
|
||
| $locale = null; |
There was a problem hiding this comment.
this does nothing, its overwritten in the next iteration.
|
not sure what exactly causes the test failure - looks unrelated to this PR. the php class is |
|
Updated |
|
Tests failing, unidentified index |
|
@dantleech this seems to be a bug in Doctrine PHPCR, when calling This PR works perfectly, but the test doesn't. Should be skip it until the bug has been fixed, so we can merge this? |
|
It depends if we are going to release CMF before PHPCR-ODM 1.3. If we have found a regression it must be fixed before the 1.3 release. If we plan to release 1.3 before or at the same time as the next CMF round of releases, I would prefer to wait. Could you create an issue on phpcr-odm? If nobody else looks I will try and tackle it on Saturday |
|
phpcr-odm 1.3 is at RC2, i would hope we can release that very soon and
before the cmf release.
|
|
Build passes now, thanks @dantleech |
|
so, should we merge this? or is doctrine/phpcr-odm#665 still a problem? |
|
No, @dantleech worked around that bug in the tests. |
Make sure locale doesn't change after flush is called
|
Cheers @wouterj |
Fixes #145
I could not find an easy way to test this...