doctrine / phpcr-odm

Doctrine PHPCR ODM
http://www.doctrine-project.org/projects/phpcr-odm.html
MIT License
183 stars 96 forks source link

QueryBuilder::setLocale has no effect for HYDRATE_DOCUMENT #605

Open uwej711 opened 9 years ago

uwej711 commented 9 years ago

in DocumentManager::getDocumentsByPhpcrQuery the locale of the query is ignored.

uwej711 commented 9 years ago

The locale is actually not even passed from the QueryBuilder to the Query, it is lost ...

dantleech commented 9 years ago

Not sure I understand. a PHPCR QueryInterface does not know anything about locales.

Why use the PHPCR query object instead of the ODM query builder?

uwej711 commented 9 years ago

From http://doctrine-phpcr-odm.readthedocs.org/en/latest/reference/query-builder.html#querying-translated-documents

uwej711 commented 9 years ago

This suggest that you just use setLocale to get your content in a different language. But this does not work, the content is always returned in the current locale.

Background: I want to allow my editors to edit foreign language without changing the language of the rest of the UI. This works in Sonata in the edit view since there I can call doLoadTranslation before showing the content. But for the list view I need to have the query return the content in the correct language.

dbu commented 9 years ago

i fear you mixed up the phpcr-odm query builder and the phpcr query builder, could that be? on phpcr level, there is indeed nothing with locales.

i think what you want to do is getLocaleChooserStrategy()->setLocale('x').

btw, i think the feature you describe sounds a lot like https://github.com/sonata-project/SonataDoctrinePhpcrAdminBundle/pull/328 - if you have some inputs there, that would be highly appreciated.

uwej711 commented 9 years ago

@dbu this is exactly where i started from, but no, I do not mix ODM and PHPCR QueryBuilder. I will try to do a failing test ...

dbu commented 9 years ago

ok, sorry. then that sounds like a bug, or a feature that we thought of but in the end did not finish to implement. glad if you can provide a functional test so we can start fixing this.

uwej711 commented 9 years ago

See failing test in #607

uwej711 commented 9 years ago

see https://github.com/valiton-forks/phpcr-odm/tree/sd_fixes_1_2_2 for a minimal fix (not really tested yet)

dbu commented 9 years ago

can you open a WIP pull request that we can comment on?

i think we should wrap $this->dm->getDocumentsByPhpcrQuery($this->query, $this->documentClass, $this->primaryAlias); into a try-catch and change the default locale while fetching documents, then change it back. otherwise we add quite some overhead i fear. and possibly destroy lazy loading.

uwej711 commented 9 years ago

I don't like the idea of setting a rather global setting just for doing this, currently it might work but I can see scenarios where this could break. Rather create a equivalent to findMany that works with locales like findTranslation. What I also miss is a shortcut for doLoadTranslation in the DocumentManager. There seems to be no other way to load different translations into the same document instance.

dbu commented 9 years ago

yeah, i see that this is not exactly elegant. maybe the getDocumentsByPhpcrQuery could pass around a locale?

the document manager has findTranslation, this should be all that is needed. findTranslation updates the already loaded document, it does not create duplicates (as documents have the same id regardless of the locale they are currently in)

uwej711 commented 9 years ago

OK, checked findTranslation. But findTranslation uses the id of the document. If I already have a document, I need the meta class to extract the id before I'm able to use findTranslation. I would prefer a DocumentManager::loadTranslation($document, $locale, $fallback) method.

dbu commented 9 years ago

or maybe we could redefine findTranslation to accept the document instead of an id as well. there is no reason why a valid document should not represent the document just as well as the id. wdyt?

dantleech commented 9 years ago

Semantically that would seem strange. "loading" is more appropriate, also it would make the first argument redundant:

$dm->findTranslation('SomeClassNameThatDoesntMatter', $document, 'de');

So I would vote for loadTranslation I think.

dbu commented 9 years ago

see also #616 - most DocumentManager methods depend on the locale returned by LocaleChooser, we should have a generic solution without exploding the number of methods.