Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Make sub classes inherit the parents referenceable property #185

Merged
merged 6 commits into from

3 participants

@dantleech
Collaborator

Hi!

I don't know if this is the best way to do this, or if I should add the other class level annotations (versionable, etc).

I see there is also a ticket PHPCR-77

@dbu
Collaborator

sounds correct to inherit this. questions:

  • what happens if i have referenceable=false in the inheriting class? is that silently ignored, or overwriting the inheritance?
  • should we semantically even allow to have a non-referenceable class extending a referenceable class? i think we should not. but we should check if the attribute in the extending class is there and throw an error to make the user understand he is trying to do something impossible.
@lsmith77
Owner

@dbu agreed.

@dbu
Collaborator

lukas, are you saying you agree we should check and make sure extending class does not become non-referenceable again?

@dantleech think you can add this check?

@lsmith77
Owner

add the check and throw an exception if someone tries to disable referenceing through inheritance

@dantleech
Collaborator

yeah, am doing it now. also trying to add a unit test, will make a pull request shortly.

@dbu
Collaborator

awesome. you can just push to your existing branch, it will update this PR. just put a comment when ready, as github does not notify us on new commits.

dantleech added some commits
dantleech Mapping exception when child doc. tries to change referenceable to false 77845a6
dantleech Merge branch 'master' into phpcr-77
Conflicts:
	lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadataFactory.php
af2f0e3
@dantleech
Collaborator

ok .. i can't get the unit test working -- it fails as the parent class doesn't seem to be "transient", and tbh I have no idea how to set it up atm.

Is this waht you had in mind?

Also managed to commit a bunch of .swp files and other junk by mistake ....

composer.json
@@ -15,7 +15,8 @@
"php": ">=5.3.2",
"doctrine/common": ">=2.3-dev,<2.4-dev",
"phpcr/phpcr-implementation": ">=2.1.0-beta3,<2.2.0-dev",
- "phpcr/phpcr-utils": ">=1.0-beta3"
+ "phpcr/phpcr-utils": ">=1.0-beta3",
+ "jackalope/jackalope-doctrine-dbal": "dev-master"
@dbu Collaborator
dbu added a note

this line should be removed again, we do not want to force a decision on the user of the lib

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
.../Tests/ODM/PHPCR/Mapping/ClassMetadataFactoryTest.php
@@ -56,4 +56,22 @@ public function testCacheDriver()
{
$this->markTestIncomplete('Test cache driver setting and handling.');
}
+MappingException
@dbu Collaborator
dbu added a note

this looks like a syntax error. can you move the expected exception insde the comment and close the comment before the test function please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadataFactory.php
@@ -201,6 +201,12 @@ private function addInheritedFields(ClassMetadata $subClass, ClassMetadata $pare
if ($parentClass->lifecycleCallbacks) {
$subClass->mapLifecycleCallbacks($parentClass->lifecycleCallbacks);
}
+
+ if ($parentClass->referenceable === true && $subClass->referenceable === false) {
+ throw MappingException::cannotOverrideReferenceableAsFalse($subClass->name);
@dbu Collaborator
dbu added a note

i think you need to add this method to the MappingException class

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

yes, this is the semantic i though of. can you please fix the syntax errors and remove the .swp files? then travis should show us what exactly the error about transient is - or maybe lsmith77 has some input on that.

@dantleech
Collaborator
@dantleech
Collaborator

I have removed all the junk from this PR, the unit tests don't work as I am not loading the Annotation driver, but in anycase I think I have taken the wrong approach.

Im guessing that I should be testing the "metadata" directly and not using the Annotation/YAML/XML drivers, possibly placing my fixtures in tests/Doctrine/Models/ODM/PHPCR/Mapping/Model/php ?

@dbu
Collaborator

i think it would be ok to test with the driver, then we know it really works. basically should be the same as when you use the annotations elsewhere. do you think you can find the problem with the test? otherwise i can have a look.

the current travis fail seems to be a typo. you can also try to run the tests locally, just run composer install --dev in the root directory of phpcr-odm. very convenient to try things and debug.

@dantleech
Collaborator

ok. I have a day off tomorrow so can spend some time getting this working. I know already that the actual test (the if statement) is in the wrong place as it checks the referenceable property BEFORE it has been set with loadClassMetadata.

dantleech Referenceable child tests
- Added annotation based tests for referenceable child Documents
c270f94
@dantleech
Collaborator

right -- I have added working tests but have removed the exception.

Currently the drivers only set referenceable to TRUE when the value of referenceable is truthy. So referenceable = false is never taken into account.

@dbu
Collaborator
dbu commented

i am ok with not checking for referenceable=false attribute and thus silently ignoring the inconsistency. thanks for this

@dbu dbu merged commit f5fb5ba into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 28, 2012
Commits on Oct 29, 2012
  1. Merge branch 'master' into phpcr-77

    dantleech authored
    Conflicts:
    	lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadataFactory.php
Commits on Oct 30, 2012
  1. Removed junk from PR

    dantleech authored
Commits on Nov 1, 2012
  1. Referenceable child tests

    dantleech authored
    - Added annotation based tests for referenceable child Documents
This page is out of date. Refresh to see the latest.
View
2  lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadataFactory.php
@@ -201,6 +201,8 @@ private function addInheritedFields(ClassMetadata $subClass, ClassMetadata $pare
if ($parentClass->lifecycleCallbacks) {
$subClass->mapLifecycleCallbacks($parentClass->lifecycleCallbacks);
}
+
+ $subClass->setReferenceable($parentClass->referenceable);
}
private function registerParentOnField(ClassMetadata $subClass, ClassMetadata $parentClass, $fieldName)
View
32 tests/Doctrine/Tests/ODM/PHPCR/Mapping/ClassMetadataFactoryTest.php
@@ -56,4 +56,36 @@ public function testCacheDriver()
{
$this->markTestIncomplete('Test cache driver setting and handling.');
}
+
+ public function testLoadMetadata_referenceableChildOverriddenAsFalse()
+ {
+ // if the child class overrides referenceable as false it is not taken into account
+ // as we only ever set the referenceable property to TRUE. This prevents us from
+ // knowing if the user has explicitly set referenceable to FALSE on a child entity.
+
+ $cache = new \Doctrine\Common\Cache\ArrayCache();
+ $reader = new \Doctrine\Common\Annotations\AnnotationReader($cache);
+ $annotationDriver = new \Doctrine\ODM\PHPCR\Mapping\Driver\AnnotationDriver($reader);
+ $annotationDriver->addPaths(array(__DIR__ . '/Model'));
+ $this->dm->getConfiguration()->setMetadataDriverImpl($annotationDriver);
+
+ $cmf = new ClassMetadataFactory($this->dm);
+ $meta = $cmf->getMetadataFor('Doctrine\Tests\ODM\PHPCR\Mapping\Model\ReferenceableChildReferenceableFalseMappingObject');
+
+ $this->assertTrue($meta->referenceable);
+ }
+
+ public function testLoadMetadata_referenceableChild()
+ {
+ $cache = new \Doctrine\Common\Cache\ArrayCache();
+ $reader = new \Doctrine\Common\Annotations\AnnotationReader($cache);
+ $annotationDriver = new \Doctrine\ODM\PHPCR\Mapping\Driver\AnnotationDriver($reader);
+ $annotationDriver->addPaths(array(__DIR__ . '/Model'));
+ $this->dm->getConfiguration()->setMetadataDriverImpl($annotationDriver);
+
+ $cmf = new ClassMetadataFactory($this->dm);
+ $meta = $cmf->getMetadataFor('Doctrine\Tests\ODM\PHPCR\Mapping\Model\ReferenceableChildMappingObject');
+
+ $this->assertTrue($meta->referenceable);
+ }
}
View
18 tests/Doctrine/Tests/ODM/PHPCR/Mapping/Model/ReferenceableChildMappingObject.php
@@ -0,0 +1,18 @@
+<?php
+
+namespace Doctrine\Tests\ODM\PHPCR\Mapping\Model;
+
+use Doctrine\ODM\PHPCR\Mapping\Annotations as PHPCRODM;
+
+/**
+ * Object which extends a referenceable object and so should also
+ * be referenceable.
+ *
+ * @PHPCRODM\Document()
+ */
+class ReferenceableChildMappingObject extends ReferenceableMappingObject
+{
+ /** @PHPCRODM\Id */
+ public $id;
+}
+
View
18 tests/Doctrine/Tests/ODM/PHPCR/Mapping/Model/ReferenceableChildReferenceableFalseMappingObject.php
@@ -0,0 +1,18 @@
+<?php
+
+namespace Doctrine\Tests\ODM\PHPCR\Mapping\Model;
+
+use Doctrine\ODM\PHPCR\Mapping\Annotations as PHPCRODM;
+
+/**
+ * An object that extends a referenceable object but sets
+ * referenceable to FALSE, which is not permitted.
+ *
+ * @PHPCRODM\Document(referenceable=false)
+ */
+class ReferenceableChildReferenceableFalseMappingObject extends ReferenceableMappingObject
+{
+ /** @PHPCRODM\Id */
+ public $id;
+}
+
Something went wrong with that request. Please try again.