query translated children #409

Merged
merged 1 commit into from Feb 28, 2014

Conversation

Projects
None yet
3 participants
Member

dbu commented Feb 1, 2014

this is really unfinished but i hope to get some input on the direction. there is dumps and die in it...

@lsmith77 i remember you where not happy with moving part of the query logic into the translation strategy, but i really don't see how to do without.

fix #402

@dantleech dantleech commented on an outdated diff Feb 1, 2014

...ine/ODM/PHPCR/Query/Builder/BuilderConverterPhpcr.php
if (!empty($this->translator[$alias]) && !empty($fieldMeta['translated'])) {
$property = $this->translator[$alias]->getTranslatedPropertyName($this->locale, $fieldMeta['property']);
+die('loo');
+ if (!isset($this->fromTranslationAlias[$alias])) {
+ die('lala');
+ $this->from = $this->translator[$alias]->alterFrom($this->from, $alias, $this->locale, $fieldMeta['property']);
@dantleech

dantleech Feb 1, 2014

Contributor

Not related, but I am not sure if we should be talking about source or from. I guess if we now talk about alias and not selector then we should also talk about from and not source.

Member

lsmith77 commented Feb 2, 2014

I am not opposed to this approach in principle and I do not see an alternative.

Member

dbu commented Feb 7, 2014

This is getting pretty far by now. I generate a query like this:

SELECT * 
FROM [nt:unstructured] AS a 
  RIGHT OUTER JOIN [nt:base] AS _en_a 
  ON ISCHILDNODE(_en_a, a) 
WHERE (_en_a.topic = 'Not Exist' 
  AND (a.[phpcr:class] = 'Doctrine\Tests\Models\Translation\ChildTranslationArticle' 
           OR a.[phpcr:classparents] = 'Doctrine\Tests\Models\Translation\ChildTranslationArticle')
  )

Now my problem is that in addition to have _en_a be a child of a, it would also need to have a specific node name. But the joins can only be done on hierarchy, not on all constraints, if i understand it correctly.

Or do you find a way to filter on the name right in the join? Otherwise, should i add yet another method to the strategy interface? or allow the alter interface to also alter the conditions (how, return array or pass references to $this->from and $this->constraint) ?

having something for the conditions is probably more future proof for other translation strategies.

@dbu dbu and 1 other commented on an outdated diff Feb 14, 2014

...ine/ODM/PHPCR/Query/Builder/BuilderConverterPhpcr.php
}
- return $property;
+ $propertyPath = $this->translator[$alias]->getTranslatedPropertyPath($alias, $fieldMeta['property'], $this->locale);
+ if (!isset($this->queryAlteredByTranslation[$alias])) {
+ $this->translator[$alias]->alterQueryForTranslation($this->qomf, $this->from, $this->constraint, $alias, $this->locale, $fieldMeta['property']);
+ $this->queryAlteredByTranslation[$alias] = true;
+ }
@dbu

dbu Feb 14, 2014

Member

this is where the stuff happens

@dantleech

dantleech Feb 19, 2014

Contributor

Can we move this into the getQuery method, and here we coiuld just do:

if (!isset($this->translatedProperties[$alias])) {
    $this->translatedProperties[$alias] = array();
}

$this->translatedproperties[$alias][] = $field;

then

// public function getQuery()
// ... after dispatching the query parts..
foreach ($this->translatedProperties as $alias => $propertyName) {
    // make stuff happen here
}

Would that help with the noPrevious problem? Is this cleaner?

@dbu dbu and 1 other commented on an outdated diff Feb 14, 2014

...ine/ODM/PHPCR/Query/Builder/BuilderConverterPhpcr.php
$res = $this->dispatch($constraint);
+ if ($noPrevious && $this->constraint) {
+ $res = $this->qomf->andConstraint($res, $this->constraint);
+ }
@dbu

dbu Feb 14, 2014

Member

i had to do this to avoid issues when we build a combined constraint, as otherwise we joined parts of the constraints with itself. feels strange however.
@dantleech if you have a better idea, please tell - the base issue is that during this eval, we don't have any $this->constraint yet, so the translation strategy will create one rather as there is no existing append it to the existing one. we should not overwrite that at the end.

@dbu

dbu Feb 14, 2014

Member

i think the walkAndWhere going to walkWhere ends up being repeated, when we have nested and wheres.

@dantleech

dantleech Feb 18, 2014

Contributor

this does seem strange. will try and have a closer look in the next few days.

@dbu

dbu Feb 19, 2014

Member

note that i pass the constraint by reference to the translation strategy and create a constraint if there is none yet. and here we use the implicit information whether the constraint is set or not...

@dantleech

dantleech Feb 19, 2014

Contributor

not sure I get this .. walkWhere is only ever called once, a query can only ever have one "where()" call, so there will never be a previous constraint -- what am I missing?

@dbu

dbu Feb 21, 2014

Member

i suspect its because of this

public function walkWhereAnd(WhereAnd $whereAnd)
{
    if (!$this->constraint) {
        return $this->walkWhere($whereAnd);
    }

so, some recursion that is biting us.

@dbu

dbu Feb 21, 2014

Member

ok, not relevant anymore as your suggestion for when to alter the query made this obsolete

@dbu dbu and 1 other commented on an outdated diff Feb 14, 2014

...ine/ODM/PHPCR/Query/Builder/BuilderConverterPhpcr.php
$meta = $this->mdf->getMetadataFor($node->getDocumentFqn());
$this->aliasMetadata[$alias] = $meta;
- if ($this->locale && 'attribute' === $meta->translator) {
+ if ($this->locale) {
@dbu

dbu Feb 14, 2014

Member

took me a moment :-)

@dantleech

dantleech Feb 19, 2014

Contributor

What would happen if translator is null?

@dbu

dbu Feb 21, 2014

Member

we go to DocumentManager::getTranslationStrategy which will throw us a nice exception.

hm, but if you join a translatable with a non-translatable document, you will be in trouble. adding a check if there is any translation strategy on the document.

@dbu dbu and 1 other commented on an outdated diff Feb 14, 2014

...ine/ODM/PHPCR/Query/Builder/BuilderConverterPhpcr.php
- if (!empty($this->translator[$alias]) && !empty($fieldMeta['translated'])) {
- $property = $this->translator[$alias]->getTranslatedPropertyName($this->locale, $fieldMeta['property']);
+ if (empty($this->translator[$alias])
+ || !$this->translator[$alias] instanceof QueryTranslationStrategyInterface
@dbu

dbu Feb 14, 2014

Member

or should we throw an exception in this case, when we don't have a query capable translation strategy?

@lsmith77

lsmith77 Feb 15, 2014

Member

would it still be possible to use the ODM QB then for translation strategies that do not support queries by somehow disabling translation support for that query instance? maybe we should then ask the user to disable the exception on a per translation basis in each query instance.

ie. $query->skipTranslationStrategy($name)

otherwise it will throw an exception

@dbu

dbu Feb 17, 2014

Member

i am quite unsure what to do. we could also have the query things in the
basic translation interface, and people can either just do noops or
throw exceptions in their strategies. its complicated enough, having
this skip exception thing makes it even weirder for a thing that you
rarely even need, as you don't do custom translation strategies normally.

Member

dbu commented Feb 14, 2014

okay, the tests succeed (at least locally). i am happy for feedback, did quite some stuff here. but in my opinion it could be merged now.

dbu added the query label Feb 15, 2014

@dbu dbu modified the milestone: 1.1 Feb 15, 2014

@dantleech dantleech and 1 other commented on an outdated diff Feb 18, 2014

...ine/ODM/PHPCR/Query/Builder/BuilderConverterPhpcr.php
- if (!empty($this->translator[$alias]) && !empty($fieldMeta['translated'])) {
- $property = $this->translator[$alias]->getTranslatedPropertyName($this->locale, $fieldMeta['property']);
+ if (empty($this->translator[$alias])
+ || !$this->translator[$alias] instanceof QueryTranslationStrategyInterface
+ || empty($fieldMeta['translated'])
+ ) {
+ return array($alias, $propertyName);
@dantleech

dantleech Feb 18, 2014

Contributor

Why do we return both the alias and the propertyName when the alias is a prerequisite for calling this method?

@dbu

dbu Feb 19, 2014

Member

the alias is the "original alias". on a child translation strategy we do a join, so we need to use a different alias (that of the joined child node) in the phpcr query. but i could indeed improve the naming.

@dantleech dantleech commented on an outdated diff Feb 18, 2014

...HPCR/Translation/AttributeTranslationStrategyTest.php
+
+class AttributeTranslationStrategyTest extends PHPCRTestCase
+{
+ public function testSetPrefixAndGetPropertyName()
+ {
+ $s = new AttributeTranslationStrategy();
+ $s->setPrefix('test');
+
+ $class = new \ReflectionClass('Doctrine\ODM\PHPCR\Translation\TranslationStrategy\AttributeTranslationStrategy');
+ $method = $class->getMethod('getTranslatedPropertyName');
+ $method->setAccessible(true);
+
+ $name = $method->invokeArgs($s, array('fr', 'field'));
+ $this->assertEquals('test:fr-field', $name);
+ }
+
@dantleech

dantleech Feb 18, 2014

Contributor

Extra line

Contributor

dantleech commented Feb 18, 2014

This seems good, though I am not yet at 100% comprehension. Would it make any sense to somehow decouple the translation logic from the query builder? DQL query hints seem like something somewhat analogous (http://docs.doctrine-project.org/en/2.0.x/cookbook/dql-custom-walkers.html).

Am I right in thinking that we modify the PHPCR query when a certain condition occurs? Would it make sense to approach this generically, like, err, maybe we want to do similar things for versioning, or for soft-deleted entities, etc?

Member

dbu commented Feb 19, 2014

the query builder is not dql, is it? so we don't have an AST to walk. we could walk the QueryBuilder node tree. walking the phpcr QOM tree is probably too late, as we would have to rebuild information like what alias is what document.

do you see a quick and simple solution? otherwise i am a bit reluctant to delay the release by another month to get this working. could we release this but make a note on the query translation strategy interface that the concept is likely to change?

Contributor

dantleech commented Feb 19, 2014

I am just putting the idea out there, this is probably fine as everything is contained in the Converter class which is where it should be. But will have a closer look tonight anyway.

@dantleech dantleech and 1 other commented on an outdated diff Feb 19, 2014

...ine/ODM/PHPCR/Query/Builder/BuilderConverterPhpcr.php
}
- return $property;
+ $propertyPath = $this->translator[$alias]->getTranslatedPropertyPath($alias, $fieldMeta['property'], $this->locale);
+ if (!isset($this->queryAlteredByTranslation[$alias])) {
+ $this->translator[$alias]->alterQueryForTranslation($this->qomf, $this->from, $this->constraint, $alias, $this->locale, $fieldMeta['property']);
@dantleech

dantleech Feb 19, 2014

Contributor

What if there are multiple translated properties on an alias? With this logic we pass the $fieldMeta['property'] to the alterQueryForTranslation method, but that woiuld only happen with the "first" call on that alias to getPhpcrproperty.

@dbu

dbu Feb 21, 2014

Member

ups. the last parameter was leftover, its not used. removed it.

the idea is that we only alter the query once per document. we could also pass everything in each call, and expect the strategy to keep track - but that would get very hard to handle if there is more than one query on the same document manager...

@dbu dbu commented on an outdated diff Feb 21, 2014

...ine/ODM/PHPCR/Query/Builder/BuilderConverterPhpcr.php
}
- return $property;
+ $propertyPath = $this->translator[$originalAlias]->getTranslatedPropertyPath($originalAlias, $fieldMeta['property'], $this->locale);
+
+ $this->queryAlteredByTranslation[$originalAlias] = true;
@dbu

dbu Feb 21, 2014

Member

@dantleech thanks for your comment. indeed if i postpone altering the query, it works a lot better. i was able to revert the funny hack in walkWhere.

Member

dbu commented Feb 21, 2014

this is now ready. i realize that on travis we see a fail for jackalope-doctrine-dbal: Jackalope\NotImplementedException: ChildNodeJoinCondition

i thought it does joins? any idea what we could do?

Contributor

dantleech commented Feb 21, 2014

I guess we simply have to skip the test if the repository does not support joins?

Member

dbu commented Feb 21, 2014

seems we implement some but not all join conditions: https://github.com/jackalope/jackalope-doctrine-dbal/blob/master/src/Jackalope/Transport/DoctrineDBAL/Query/QOMWalker.php#L341
which seems a bit ridiculus, given that we implement the descendant node join condition.

hrmpf. just skipping the test is not helping that much, as people will run into the issue when using phpcr-odm with jackalope-doctrine-dbal.

Member

dbu commented Feb 21, 2014

i think it would be as simple as

    return "($rightAlias.path LIKE CONCAT($leftAlias.path, '/%')) AND $leftAlias.depth + 1 = $rightAlias.depth";

or not? can we add that and release 1.1.1? i am running out of time for this week, but sounds doable to me.

@dantleech dantleech and 1 other commented on an outdated diff Feb 22, 2014

...ine/ODM/PHPCR/Query/Builder/BuilderConverterPhpcr.php
@@ -62,6 +63,14 @@ class BuilderConverterPhpcr
protected $sourceDocumentNodes;
/**
+ * To keep track for which alias the translation strategy already got the
@dantleech

dantleech Feb 22, 2014

Contributor

Would this be more accurate?

Used to keep track of which sources will be modified by the translation strategy if applicable.
@dbu

dbu Feb 27, 2014

Member

yep, forgot to update this. thanks.

@dantleech dantleech commented on an outdated diff Feb 22, 2014

...ine/ODM/PHPCR/Query/Builder/BuilderConverterPhpcr.php
@@ -197,6 +219,10 @@ public function getQuery(QueryBuilder $builder)
}
}
+ foreach ($this->queryAlteredByTranslation as $alias => $bool) {
@dantleech

dantleech Feb 22, 2014

Contributor

$bool is redundant right? We can just do

foreach (array_keys($this->queryAlteredByTranslation) as $alias) {
}

@dantleech dantleech commented on the diff Feb 22, 2014

...ine/ODM/PHPCR/Query/Builder/BuilderConverterPhpcr.php
$property->getAlias(),
$property->getField()
);
$column = $this->qomf->column(
- $property->getAlias(),
- // do we want to support custom column names in ODM?
@dantleech

dantleech Feb 22, 2014

Contributor

The question still stands. I guess the use case is for when we hydrate to an array.

@dbu

dbu Feb 27, 2014

Member

i re-added the comment.

@dantleech dantleech commented on an outdated diff Feb 22, 2014

...tion/TranslationStrategy/ChildTranslationStrategy.php
+ ) {
+ $childAlias = "_{$locale}_{$alias}";
+
+ $selector = $qomf->join(
+ $selector,
+ $qomf->selector($childAlias, 'nt:base'),
+ QueryObjectModelConstantsInterface::JCR_JOIN_TYPE_RIGHT_OUTER,
+ $qomf->childNodeJoinCondition($childAlias, $alias)
+ );
+
+ $languageConstraint = $qomf->comparison(
+ $qomf->nodeName($childAlias),
+ QueryObjectModelConstantsInterface::JCR_OPERATOR_EQUAL_TO,
+ $qomf->literal(Translation::LOCALE_NAMESPACE . ":$locale")
+ );
+ if ($constraint) {
@dantleech

dantleech Feb 22, 2014

Contributor

I always put an empty line before if if it is not the first line of a method.

@dantleech dantleech commented on the diff Feb 22, 2014

...nctional/Translation/ChildTranslationStrategyTest.php
@@ -349,4 +350,95 @@ public function testTranslationArrayProperties()
'url' => 'super-article-en-francais.html'
), $doc->getSettings());
}
+
+ public function testQueryBuilder()
+ {
+ $strategy = new ChildTranslationStrategy();
+ $this->dm->setTranslationStrategy('children', $strategy);
+ $this->dm->setLocaleChooserStrategy(new LocaleChooser(array('en' => array('fr'), 'fr' => array('en')), 'en'));
+
+ // First save some translations
+ $data = array();
+ $data['topic'] = 'Some interesting subject';
+ $data['text'] = 'Lorem ipsum...';
+ $data['settings'] = array(
+ 'is-active' => 'true',
+ 'url' => 'great-article-in-english.html'
@dantleech

dantleech Feb 22, 2014

Contributor

Inconsistent CS

@dbu

dbu Feb 27, 2014

Member

this is just tests :-)
but what do you mean? the tests above have the same cs, i was just copying things. (i know, lame excuse)

@dantleech

dantleech Feb 27, 2014

Contributor

Oh, I didn't see the other lines - but it seems inconsistent with all the other code I have worked with.

@dbu

dbu Feb 27, 2014

Member

right. i propose we clean that up in a separate commit outside of this PR, ok?

@dantleech

dantleech Feb 27, 2014

Contributor

pas de problème :) maybe we can experiment with scrutinizer or phpcs-fixer

@dantleech dantleech commented on the diff Feb 22, 2014

...nctional/Translation/ChildTranslationStrategyTest.php
+ 'is-active' => 'true',
+ 'url' => 'great-article-in-english.html'
+ );
+
+ $node = $this->getTestNode();
+ $node->setProperty('author', 'John Doe');
+ $node->setProperty('phpcr:class', 'Doctrine\Tests\Models\Translation\ChildTranslationArticle');
+
+ $strategy->saveTranslation($data, $node, $this->metadata, 'en');
+
+ // Save translation in another language
+
+ $data['topic'] = 'Un sujet intéressant';
+ $data['settings'] = array(
+ 'is-active' => 'true',
+ 'url' => 'super-article-en-francais.html'
@dantleech

dantleech Feb 22, 2014

Contributor

Same.

@dantleech dantleech commented on an outdated diff Feb 22, 2014

...nctional/Translation/ChildTranslationStrategyTest.php
+ $res = $qb->getQuery()->execute();
+ $this->assertCount(1, $res);
+
+ $qb = $this->dm->createQueryBuilder();
+ $qb->setLocale('fr');
+ $qb->from()->document('Doctrine\Tests\Models\Translation\ChildTranslationArticle', 'a');
+ $qb->where()
+ ->eq()
+ ->field('a.topic')
+ ->literal('Un sujet intéressant')
+ ->end();
+
+ $res = $qb->getQuery()->execute();
+ $this->assertCount(1, $res);
+ }
+
@dantleech

dantleech Feb 22, 2014

Contributor

One line too many.

@dantleech dantleech commented on an outdated diff Feb 22, 2014

...tion/TranslationStrategy/ChildTranslationStrategy.php
{
return $fieldName;
}
+
+ /**
+ * {@inheritDoc}
+ *
+ * We need to select the field on the joined child node.
+ */
+ public function getTranslatedPropertyPath($alias, $propertyName, $locale)
+ {
+ $childAlias = "_{$locale}_{$alias}";
@dantleech

dantleech Feb 22, 2014

Contributor

This would be the first time this CS would be used in PHPCR-ODM. Dot concatenation seems the most popular.

$var = '_' . $locale . '_' . $alias;
$var = '_'.$locale.'_'.$alias;
$var = sprintf('_%s_%s', $locale, $alias);

I prefer sprintf.

Member

dbu commented Feb 25, 2014

i created a PR to implement the required join condition in jackalope-doctrine-dbal: jackalope/jackalope-doctrine-dbal#177

please review - i will then address the CS issues.

Member

dbu commented Feb 27, 2014

updated. and tests seem green!

@dbu dbu and 2 others commented on an outdated diff Feb 27, 2014

...ine/ODM/PHPCR/Query/Builder/BuilderConverterPhpcr.php
- if (!empty($this->translator[$alias]) && !empty($fieldMeta['translated'])) {
- $property = $this->translator[$alias]->getTranslatedPropertyName($this->locale, $fieldMeta['property']);
+ if (empty($fieldMeta['translated'])
+ || empty($this->translator[$originalAlias])
+ || !$this->translator[$originalAlias] instanceof QueryTranslationStrategyInterface
@dbu

dbu Feb 27, 2014

Member

okay, getting there. last question: should we keep this QueryTranslationStrategyInterface or simply have it in the standard translator interface?

@dantleech

dantleech Feb 28, 2014

Contributor

Are there any acual cases where queries are not supported? if not I would vote to make it mandatory and maybe reconsider in the future if ever the need arises.

@lsmith77

lsmith77 Feb 28, 2014

Member

well the only two known strategies are created by us and would now be supported. however lets say someone creates a google translate strategy, they would likely not be able to support querying.

@dbu

dbu Feb 28, 2014

Member

@lsmith77 i tend to say we make the query functions mandatory to translation strategies. a strategy can still decide to do nothing to ignore the problem, or throw an exception to tell the user he can not query that field. silently failing is not helpful i think.

is that ok?

@lsmith77

lsmith77 Feb 28, 2014

Member

fine for me .. mostly because i assume there aren't any people who did their own strategies and therefore nobody will suffer from the BC break

Member

dbu commented Feb 28, 2014

okay. if travis agrees, this is now good to merge!

@dbu dbu added a commit that referenced this pull request Feb 28, 2014

@dbu dbu Merge pull request #409 from doctrine/query-children
query translated children
7968e8f

@dbu dbu merged commit 7968e8f into master Feb 28, 2014

1 check passed

default Scrutinizer: 2 new/changed issues, 9 added/modified code elements — Travis: Passed
Details

dbu deleted the query-children branch Feb 28, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment