Retrieve locales for non-persisted documents

[dantleech]

Hacky fix to retrieve locales for not-yet-in-storage documents. Currently if you try getLocalesFor on a new and persisted document the UOW will call the session to get the node which will throw a "node not found in workspace" exception.

This PR adds a try catch - but this should not be necessary.

Basically we just need to determine if this document has been loaded by the UOW - but there is no STATE_* constant that corresponds to this -- is there a better way to do this today?

[dantleech]

Infact, I remember having a similar discussion about this a while back with @dbu when I had to add the following code to determine if a document existed.

    protected function documentIsPersisted($document)
    {
        $metadata = $this->dm->getClassMetadata(get_class($document));
        $id = $metadata->getIdentifierValue($document);
        $phpcrSession = $this->dm->getPhpcrSession();
        return $phpcrSession->nodeExists($id);
    }

So its a question of if the UnitOfWork should be able to determine if a document has actually exists in storage - I think it should.

[lsmith77]

there is UnitOfWork::getDocumentState()

[dantleech]

That does not tell you if the document is persisted or not. It only says
"NEW", "MANAGED", etc.. MANAGED means that "persist" has been called,
not that it has been comitted to the database.

On Sat, Oct 26, 2013 at 08:26:35AM -0700, Lukas Kahwe Smith wrote:

there is UnitOfWork::getDocumentState()

—
Reply to this email directly or [1]view it on GitHub.

References

Visible links

1. Retrieve locales for non-persisted documents #372 (comment)

[lsmith77]

Ah, I think now I remember the previous discussion. However I am not sure if documentIsPersisted() makes sense semantically .. to me documentIsPersisted() would mean that a document (ie. a class instance) was persisted there .. but this would return true even if just a node was created and no document was ever persisted. also I am starting to be concerned about the quantity of methods we have on the DM. Maybe we should provide a more or less "stateless" helper class?

[dantleech]

Yeah, sorry, the above code was just to illustrate where I had to work around this - a better method name should probably use the word "comitted", e.g. documentIsComitted - but this is actually not what this PR is about.

This PR adds a try/catch to getLocalesFor($document) - I see now that I could also call $session->nodeExists to avoid this - but it should be possible to avoid even that? e.g. the UOW should be aware that it has not /retrieved/ the document.

[dbu]

but this is a lie, right? i mean the document will currently have a locale, which it will have once flushed. and if we previously called bindTranslation, there might even exist several translations. you could try to look into $this->documentTranslations[$oid] which has a list of currently bound locales (and then the actual fields under each locale).

[dbu]

messing with the states is difficult, and starting to have some sort of sub-state is tricky as well. when looking into computeChangeSet it seems we use $isNew = !isset($this->originalData[$oid]); - this is so simple its not really worth to extract it into a method. you could use this check instead of $session->nodeExists, that avoids a round trip to phpcr. that instead of try-catch would make this good enough for me. (but see also my comment on what we do when actually there is a new document)

[dantleech]

Actually this really hits the wall with:

symfony-cmf/routing-auto-bundle#33

Basically I need not only to get the locales, but the translated fields also before flush and the UOW makes my head spin round.

[dbu]

oh, i see. i think it would be the correct behaviour for both findTranslation and getLocalesFor to work on unflushed documents. find(null, '/path') works if the document is existing but not flushed, right? then it would be logical for the others to work as well.

all translations are stored in $this->documentTranslations by oid and locale. this should be usable to restore the document to a different translation.

[dantleech]

Replaced by : #375