Make sub classes inherit the parents referenceable property #185

Merged
merged 6 commits into from Nov 1, 2012

Projects

None yet

3 participants

Contributor

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

Member
dbu commented Oct 29, 2012

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.
Member

@dbu agreed.

Member
dbu commented Oct 29, 2012

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?

Member

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

Contributor

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

Member
dbu commented Oct 29, 2012

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 Oct 29, 2012
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
Contributor

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 ....

@dbu dbu commented on an outdated diff Oct 29, 2012
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
dbu Oct 29, 2012 Member

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

@dbu dbu commented on an outdated diff Oct 29, 2012
.../Tests/ODM/PHPCR/Mapping/ClassMetadataFactoryTest.php
@@ -56,4 +56,22 @@ public function testCacheDriver()
{
$this->markTestIncomplete('Test cache driver setting and handling.');
}
+MappingException
dbu
dbu Oct 29, 2012 Member

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

@dbu dbu commented on an outdated diff Oct 29, 2012
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
dbu Oct 29, 2012 Member

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

Member
dbu commented Oct 29, 2012

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.

Contributor

Yeah, no problem. Will revise the PR tomorrow.

Envoyé de mon téléphone Android avec K-9 Mail. Excusez la brièveté.

David Buchmann notifications@github.com a écrit :

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.


Reply to this email directly or view it on GitHub.

Contributor

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 ?

Member
dbu commented Oct 31, 2012

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.

Contributor

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
Contributor

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.

Member
dbu commented Nov 1, 2012

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 doctrine:master Nov 1, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment