Skip to content

Fix parent id order #120

Merged
merged 5 commits into from Mar 9, 2012

4 participants

@uwej711
uwej711 commented Mar 8, 2012

fixes an rather old issue ...

@lsmith77
Doctrine member
lsmith77 commented Mar 8, 2012

this means if there is a parent mapping the parent strategy must be used?

@uwej711
uwej711 commented Mar 8, 2012

Yes - it was designed like that but did only work when the order of the annotations was first Id then ParentDocument.
Of course there are use cases where you want to map the parent but don't want to use ParentIdStrategy, but this is not addressed here. Either remove the magic completely and force the developer to specify the strategy or add a flag to the ParantDocument annotation ...

@lsmith77
Doctrine member
lsmith77 commented Mar 8, 2012

ok .. i understand the problem now. i think we have to find a cleaner way to handle this.

in general the behavior is fine like it is, unless you have with an Id and a ParentDocument mapping, right? or generally we need to make it possible to force a specific strategy according to the users wishes and never map multiple id strategy's

so imho the best solution is to ensure that if there are multiple mappings, the one defined inside the id always wins. meaning that if one maps with an Id and a ParentDocument but wants to use the parent id strategy, that needs to be configured in the Id mapping.

@uwej711
uwej711 commented Mar 8, 2012

Well for the ParentIdStrategy to work the parent needs to map the id ...

Is there a point during the mapping, where we can be sure that all annotations are mapped and then decide on what strategy to use? Without that it's a bit more complicated ...

@lsmith77
Doctrine member
lsmith77 commented Mar 8, 2012

"Well for the ParentIdStrategy to work the parent needs to map the id ..."

no .. it needs the NodeName, not the id.

i don't think there is .. but i am not 100% sure (@Stof, @beberlei ?)

but i think it should just work by checking for any mapping other than Id mappings if a strategy is already set and if there is one set, not try to override the mapping.

@uwej711
uwej711 commented Mar 8, 2012

Sorry my mistake ...

But I just realised that the ParentIdGenerator works as an AssignedIdGenerator when either ParentDocument or Nodename is not assigned.

Once again, this PR only fixes the issue that the ParentIdStrategy was not used when the id was mapped after the parentDocument ...

@lsmith77
Doctrine member
lsmith77 commented Mar 8, 2012

yeah .. but what if someone wants to use the repository id generator? i think the explicitly mapping should always win ..

@dbu
Doctrine member
dbu commented Mar 8, 2012

i think the original idea was: if you have an explicit strategy, it will win.

if there is no explicit, we take the parent strategy which will fall back to assigned id.

@uwej711 can't you switch the if's and first check for explicit strategy and then continue with the else if about parent?

@dbu dbu commented on the diff Mar 8, 2012
...Doctrine/Tests/ODM/PHPCR/Functional/HierarchyTest.php
@@ -225,14 +225,14 @@ function testProxyForChildIsUsed()
*/
class NameDoc
{
+ /** @PHPCRODM\ParentDocument */
+ public $parent;
@dbu
Doctrine member
dbu added a note Mar 8, 2012

is the order now still relevant or not anymore?

@uwej711
uwej711 added a note Mar 8, 2012

Goal of the change is to make the order irrelevant

@stof
Doctrine member
stof added a note Mar 8, 2012

but if the order is irrelevant, why do you have to change it in the test fixture ?

@lsmith77
Doctrine member
lsmith77 added a note Mar 8, 2012

to highlight that the issue is fixed ..

@uwej711
uwej711 added a note Mar 8, 2012

Because changing the order showed the presence of the issue before the mapping code was changed ...

@stof
Doctrine member
stof added a note Mar 9, 2012

@lsmith77 what really highlights that the issue is gone is the next testcase where different order are used. Having only one example could mean that the issue is still there but with the reverse order :p

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof
Doctrine member
stof commented Mar 8, 2012

@lsmith77 why are you pinging me ? I have no idea how ids are handled in PHPCR :)

@lsmith77
Doctrine member
lsmith77 commented Mar 8, 2012

i was pinging in regards to more generally how the mapping process in Doctrine works .. but yeah probably too specific as all of this isn't part of common yet. so never mind .. sorry for the noise :)

@stof
Doctrine member
stof commented Mar 8, 2012

Well, I don't see an equivalent to ParentDocument in the ORM (it is storing hierarchical data in PHPCR, right ?) So there is no such strategy

@uwej711
uwej711 commented Mar 8, 2012

I will see how hard it is ... just before I start a short summary:
1. if there is a ParentDocument and a Nodename but no id strategy set that to be ParentIdStrategy
2. if there is Nodename or ParentDocument missing and no id strategy set that to assigned
3. otherwise use given id strategy.
OK?

uwej711 added some commits Mar 8, 2012
@uwej711 uwej711 Implement logic for idGeneratorStrategy
- if parentdocument and nodename are supplied and no extra id strategy is specified use parentidstrategy
- if parentdocument and nodename and id strategy is specified use specified strategy
- otherwise use specified id strategy or use assigned id strategy as default
fabede6
@uwej711 uwej711 fix wrong mapping eae2f28
@lsmith77
Doctrine member
lsmith77 commented Mar 8, 2012

yes sounds good to me

@uwej711 uwej711 commented on the diff Mar 8, 2012
...ODM/PHPCR/Mapping/Annotations/DoctrineAnnotations.php
@@ -84,7 +84,7 @@ class TranslatableProperty extends Property
public $id = true;
public $type = 'string';
/** @var string */
- public $strategy = 'assigned';
+ public $strategy;
@uwej711
uwej711 added a note Mar 8, 2012

This is necessary since otherwise we never know whether a strategy was set by the developer or not. This was actually different from YAML and XML mapping. Internally we still default to assigned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77
Doctrine member
lsmith77 commented Mar 8, 2012

though i would order it differently

  1. if there is an id with a strategy always use that
  2. if not and there is a parent and a node name mapping use the parent strategy
  3. else use assigned strategy
@uwej711
uwej711 commented Mar 8, 2012

Yes, that's what the code is now supposed to do. It could have been coded your way if there were a method to "finalize" the mapping (well we actually could add that ...). Now it is a bit twisted ...

@stof stof commented on an outdated diff Mar 8, 2012
lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadata.php
@@ -487,13 +502,19 @@ public function mapNodename(array $mapping)
{
$this->validateAndCompleteFieldMapping($mapping, false);
$this->nodename = $mapping['fieldName'];
- }
+ if (null !== $this->parentMapping && !$this->idStrategySet)
+ {
@stof
Doctrine member
stof added a note Mar 8, 2012

CS issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Mar 8, 2012
lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadata.php
@@ -487,13 +502,19 @@ public function mapNodename(array $mapping)
{
$this->validateAndCompleteFieldMapping($mapping, false);
$this->nodename = $mapping['fieldName'];
- }
+ if (null !== $this->parentMapping && !$this->idStrategySet)
+ {
+ $this->setIdGenerator(self::GENERATOR_TYPE_PARENT);
+ }}
@stof
Doctrine member
stof added a note Mar 8, 2012

CS issue here. The curly brace of the method should be on its own line (and indented one level less)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Mar 8, 2012
lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadata.php
public function mapParentDocument(array $mapping)
{
$this->validateAndCompleteFieldMapping($mapping, false);
$this->parentMapping = $mapping['fieldName'];
- $this->setIdGenerator(self::GENERATOR_TYPE_PARENT);
+ if (null !== $this->nodename && !$this->idStrategySet)
+ {
@stof
Doctrine member
stof added a note Mar 8, 2012

issue here too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu
Doctrine member
dbu commented Mar 9, 2012

finalizing the mapping could have the added benefit of combined validation. for example if assigned strategy is parent and name but there is no field mapped to the name, it would tell this is invalid.

but if it works like this and lukas has no objections, i'd say lets merge as it is better than what we have now and create a ticket for the improvement.

@lsmith77
Doctrine member
lsmith77 commented Mar 9, 2012

looks good to me .. i assume the tests pass.

@uwej711
uwej711 commented Mar 9, 2012

Yes, they passed yesterday evening :-)

@lsmith77 lsmith77 merged commit 7625489 into doctrine:master Mar 9, 2012
@dbu
Doctrine member
dbu commented Mar 9, 2012

created http://www.doctrine-project.org/jira/browse/PHPCR-60 so that we don't forget the issue.

thanks uwe!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.