cleanup id strategy logic and validation step #351

Merged
merged 1 commit into from Oct 2, 2013

3 participants

@dbu
Doctrine member
dbu commented Oct 2, 2013

this involves a small BC break because if the parent is mapped but no nodename, we now prefer the (recently added) autoname strategy over the assigned id. the id priority is now:

  • explicit strategy defined on the id mapping
  • parent strategy if parent and nodename are both mapped
  • autoname strategy if the parent is mapped
  • assigned id strategy if the id is mapped
  • in any other situations, things can not work and an exception is thrown

we might want to update the documentation with this as well. if it al makes sense, i can do that.

fix PHPCR-60

@dbu dbu commented on an outdated diff Oct 2, 2013
lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadata.php
@@ -101,10 +105,10 @@ class ClassMetadata implements ClassMetadataInterface
*
* @var \Doctrine\ODM\PHPCR\Id\IdGenerator
*/
- public $idGenerator = self::GENERATOR_TYPE_ASSIGNED;
+ public $idGenerator = self::GENERATOR_TYPE_NONE;
@dbu
Doctrine member
dbu added a note Oct 2, 2013

having assigned id strategy without an id mapping makes no sense. we want to detect this situation.

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

sounds good to me.

@dbu dbu commented on the diff Oct 2, 2013
lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadata.php
@@ -101,14 +105,7 @@ class ClassMetadata implements ClassMetadataInterface
*
* @var \Doctrine\ODM\PHPCR\Id\IdGenerator
*/
- public $idGenerator = self::GENERATOR_TYPE_ASSIGNED;
-
- /**
- * keep track whether an id strategy was explicitly set
- *
- * @var boolean
- */
- private $idStrategySet = false;
@dbu
Doctrine member
dbu added a note Oct 2, 2013

i moved the autodetect into the validation step at the end of the mapping. that way we do not need to track this and the code is simplifiied. and the logic that defines what strategy is chosen is all in one place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on the diff Oct 2, 2013
...DM/PHPCR/Functional/Mapping/AnnotationMappingTest.php
@@ -238,18 +250,6 @@ class RepositoryIdStrategy
}
/**
-* @PHPCRODM\Document
-*/
-class AutoAssignedIdStrategy
@dbu
Doctrine member
dbu added a note Oct 2, 2013

this was the same as ParentIdStrategy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on the diff Oct 2, 2013
...M.PHPCR.Mapping.Model.TranslatorMappingObject.dcm.xml
@@ -4,6 +4,7 @@
xsi:schemaLocation="http://doctrine-project.org/schemas/phpcr-odm/phpcr-mapping
https://github.com/doctrine/phpcr-odm/raw/master/doctrine-phpcr-odm-mapping.xsd">
<document name="Doctrine\Tests\ODM\PHPCR\Mapping\Model\TranslatorMappingObject" translator="attribute">
+ <id name="id" />
@dbu
Doctrine member
dbu added a note Oct 2, 2013

we now detect missing id mappings...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on the diff Oct 2, 2013
tests/Doctrine/Tests/ODM/PHPCR/UnitOfWorkTest.php
@@ -47,6 +47,7 @@ public function setUp()
$metadata = new ClassMetadata($this->type);
$metadata->initializeReflection($cmf->getReflectionService());
$metadata->mapId(array('fieldName' => 'id', 'id' => true));
+ $metadata->idGenerator = ClassMetadata::GENERATOR_TYPE_ASSIGNED;
@dbu
Doctrine member
dbu added a note Oct 2, 2013

this is a unit test of something else. the generator is no longer set in mapId. i set it explicitly as i want to test the unitofwork, not the classmetadata

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

ok, lets see if travis agrees - i think this is now ready.

@dbu
Doctrine member
dbu commented Oct 2, 2013

i updated the doc in doctrine/phpcr-odm-documentation@3682e7e

@lsmith77 do you want to merge? i will be offline in a bit until tonight. it would be great if you could test the cmf bundles with the latest phpcr-odm prior to tagging it...

@lsmith77 lsmith77 merged commit c01f784 into master Oct 2, 2013

1 check passed

Details default The Travis CI build passed
@lsmith77 lsmith77 deleted the finalize-mapping-cleanup branch Oct 2, 2013
@dantleech

@dbu I think that the failure in symfony-cmf/menu-bundle#142 is caused by this merge.

In the DataFixtures there I am explicity assigning an ID before committing, the ODM is then complaining

Doctrine\ODM\PHPCR\PHPCRInvalidArgumentException: Parameter $document needs to be an object, NULL given

/home/daniel/www/symfony-cmf/MenuBundle/vendor/doctrine/phpcr-odm/lib/Doctrine/ODM/PHPCR/DocumentManager.php:1186
/home/daniel/www/symfony-cmf/MenuBundle/vendor/doctrine/phpcr-odm/lib/Doctrine/ODM/PHPCR/Id/AutoIdGenerator.php:41
/home/daniel/www/symfony-cmf/MenuBundle/vendor/doctrine/phpcr-odm/lib/Doctrine/ODM/PHPCR/UnitOfWork.php:1432
/home/daniel/www/symfony-cmf/MenuBundle/vendor/doctrine/phpcr-odm/lib/Doctrine/ODM/PHPCR/UnitOfWork.php:612
/home/daniel/www/symfony-cmf/MenuBundle/vendor/doctrine/phpcr-odm/lib/Doctrine/ODM/PHPCR/UnitOfWork.php:583
/home/daniel/www/symfony-cmf/MenuBundle/vendor/doctrine/phpcr-odm/lib/Doctrine/ODM/PHPCR/DocumentManager.php:616
/home/daniel/www/symfony-cmf/MenuBundle/Tests/Resources/DataFixtures/PHPCR/LoadMenuData.php:129
/home/daniel/www/symfony-cmf/MenuBundle/Tests/Resources/DataFixtures/PHPCR/LoadMenuData.php:46
/home/daniel/www/symfony-cmf/MenuBundle/vendor/doctrine/data-fixtures/lib/Doctrine/Common/DataFixtures/Executor/AbstractExecutor.php:121
/home/daniel/www/symfony-cmf/MenuBundle/vendor/doctrine/data-fixtures/lib/Doctrine/Common/DataFixtures/Executor/PHPCRExecutor.php:65
/home/daniel/www/symfony-cmf/MenuBundle/vendor/symfony-cmf/testing/src/Symfony/Cmf/Component/Testing/Functional/DbManager/PHPCR.php:52
/home/daniel/www/symfony-cmf/MenuBundle/Tests/WebTest/Voter/BaseTestCase.php:13

Where $document is a parent document. I am guessing that because this is a "new" document with an assigned ID, the parent field is NULL and so it crashes.

@dbu
Doctrine member
dbu commented Oct 6, 2013

fixed in #355

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