Skip to content

Mapping test improvements #144

Merged
merged 37 commits into from Jun 15, 2012

6 participants

@craigmarvelley

As discussed at the Symfony Live hackday, I've improved the test coverage of the XML and YAML mapping drivers and fixed a couple of bugs relating to the types of the mapped fields as stored in the CR in the process. There's still much that needs to be covered relating to the more advanced mapped properties, and I'm aiming to complete that in the coming days, but I'll open this PR now just in case you want these changes earlier.

Relates to http://www.doctrine-project.org/jira/browse/PHPCR-42

craig added some commits Jun 9, 2012
craig Moved utility method to abstract test class for use in other driver t…
…ests
ba511e6
craig Removed utility class b92b6b2
craig Added XmlDriverTest a197b02
craig Made testLoadMetadataForNonDocumentThrowsException and testGetAllClas…
…sNamesIsIdempotent tests abstract
0120fd5
craig Maintain mapping exception data when rethrowing exception 97c90b6
craig Added CmsAddress XML mapping file fe8050a
craig Made testGetAllClassNamesReturnsAlreadyLoadedClassesIfAppropriate an …
…abstract mapping test
1b0ed9c
craig Made testGetAllClassNamesReturnsOnlyTheAppropriateClasses an abstract…
… mapping test
84c4c14
craig Relocated method for visibility f1f6ec7
craig Made loadDriverForCMSDocuments an abstract method in the base class c42d5d0
craig Added YamlMappingTest which extends the AbstractMappingDriverTest class 3e46d76
craig Use a specific class for the testing of property methods. Fixed bug w…
…ith type mapping of decimal fields in XML and YAML mapping drivers
5ea8cd3
craig Use 'field' rather than 'property' to make tests less ambiguous 381ce08
craig Ensure types are correct, and unsupported field types map to their al…
…iases
514205d
craig Added tests for Nodename mapping to abstract driver class e06ef56
craig Added tests for ParentDocument mapping to abstract driver class 766ef00
craig Removed redundant files 40aae5a
craig Refactored abstract test class to remove repitition 3bbd585
craig Added tests for Child mapping to abstract driver class 82e6dc8
craig Added tests for Children mapping to abstract driver class fec02f4
craig Throw an exception if the fieldName of a children field is blank 41039a4
craig Removed redundant tests fb0cce4
craig Removed unused file b6bf172
craig Singularized Models directory for consistency 5e9c90c
craig Added tests for specifying a Repository strategy to abstract driver c…
…lass
35b9990
@lsmith77 lsmith77 and 1 other commented on an outdated diff Jun 11, 2012
lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadata.php
@@ -581,9 +581,13 @@ protected function validateAndCompleteReferrersMapping($mapping)
protected function validateAndCompleteFieldMapping($mapping, $isField = true)
{
- if (!isset($mapping['fieldName'])) {
+ if (!isset($mapping['fieldName']) || empty($mapping['fieldName'])) {
@lsmith77
Doctrine member
lsmith77 added a note Jun 11, 2012

this makes !isset() redundant .. but did you really mean to also ignore 0 ?

Originally blank strings were being accepted here, and I assume that's not a valid case. Maybe strlen($mapping['fieldName']) === 0 is better? I'm not really sure what the valid cases are to be honest.

@lsmith77
Doctrine member
lsmith77 added a note Jun 11, 2012

yeah .. 0 probably also doesn't make sense .. so i am fine if you just remove the !isset() part since its redundant now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77 lsmith77 commented on an outdated diff Jun 11, 2012
lib/Doctrine/ODM/PHPCR/Mapping/Driver/XmlDriver.php
@@ -50,7 +50,14 @@ public function __construct($locator, $fileExtension = self::DEFAULT_FILE_EXTENS
*/
public function loadMetadataForClass($className, ClassMetadata $class)
{
- $xmlRoot = $this->getElement($className);
+ try {
+ $xmlRoot = $this->getElement($className);
+ }
+ catch(\Doctrine\Common\Persistence\Mapping\MappingException $e) {
@lsmith77
Doctrine member
lsmith77 added a note Jun 11, 2012

missing a space after catch

@lsmith77
Doctrine member
lsmith77 added a note Jun 11, 2012

oh and we usually try to add a use statement to avoid fully qualified names inside the class code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77 lsmith77 commented on an outdated diff Jun 11, 2012
lib/Doctrine/ODM/PHPCR/Mapping/Driver/YamlDriver.php
@@ -50,7 +50,14 @@ public function __construct($locator, $fileExtension = self::DEFAULT_FILE_EXTENS
*/
public function loadMetadataForClass($className, ClassMetadata $class)
{
- $element = $this->getElement($className);
+ try {
+ $element = $this->getElement($className);
+ }
+ catch(\Doctrine\Common\Persistence\Mapping\MappingException $e) {
@lsmith77
Doctrine member
lsmith77 added a note Jun 11, 2012

see above notes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77 lsmith77 commented on an outdated diff Jun 11, 2012
lib/Doctrine/ODM/PHPCR/Mapping/Driver/YamlDriver.php
@@ -84,7 +91,18 @@ public function loadMetadataForClass($className, ClassMetadata $class)
}
}
if (isset($element['id'])) {
- $mapping = array('fieldName' => $element['id'], 'id' => true);
+ if(is_array($element['id'])) {
@lsmith77
Doctrine member
lsmith77 added a note Jun 11, 2012

here again a few CS issues with missing spaces and code that should be on the same line as the closing curly bracket

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

thx for working on this!

@craigmarvelley

No worries, glad to help. I'll address the comments you raised, thanks for the review :)

@stof stof and 2 others commented on an outdated diff Jun 11, 2012
lib/Doctrine/ODM/PHPCR/Mapping/Driver/XmlDriver.php
@@ -50,7 +50,14 @@ public function __construct($locator, $fileExtension = self::DEFAULT_FILE_EXTENS
*/
public function loadMetadataForClass($className, ClassMetadata $class)
{
- $xmlRoot = $this->getElement($className);
+ try {
+ $xmlRoot = $this->getElement($className);
+ }
+ catch(\Doctrine\Common\Persistence\Mapping\MappingException $e) {
+ // Convert Exception type for consistency with other drivers
+ throw new MappingException($e->getMessage(), $e->getCode(), $e->getPrevious());
@stof
Doctrine member
stof added a note Jun 11, 2012

IMO, you should keep the common MappingException as previous exception instead of stripping it

OK.. I added it because the AnnotationDriver throws the specific exception, whereas the XML and YAML drivers throw the common one. I thought it'd be more consistent and easier to test if they all threw the same type, but happy to go with the consensus.

@stof
Doctrine member
stof added a note Jun 12, 2012

I was just talking about what you put as previous exception here. But anyway, throwing the common exception is a better idea IMO (or making the MappingException extend the common one) so that some code can catch the common one if it wants to work for any Doctrine project

Ah, I misunderstood. Yep - ATM Doctrine\ODM\PHPCR\Mapping\MappingException and Doctrine\Common\Persistence\Mapping\MappingException are not related. Was trying to prevent calling code having to be aware of the driver implementation, which spoils the pattern.

@dbu
Doctrine member
dbu added a note Jun 12, 2012

is there any good reason to have a phpcr specific MappingException? i would assume that it is just that at the time of writing, the exception was not yet moved into the doctrine common but copied for each odm and the orm.

@stof i see the orm still has its own MappingException https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Mapping/MappingException.php and it extends from ORMException rather than the common MappingException - is that subject to change? is the exception concept to have a specific tree for each of the backends or should we push that all into common and eliminate the impl specific exceptions? (unless there is some additional info to track in an exception, where you would extend the common exception rather than some sort of base implemenation exception)

@stof
Doctrine member
stof added a note Jun 12, 2012

Improving the ORM exceptions is in the roadmap. Currently, they all extend from the ORMException class, which should probably be changed to an interface instead to be able to extend from other exceptions too

@dbu
Doctrine member
dbu added a note Jun 12, 2012

@stof is there a jira issue or something describing what you want to do? i guess we should do the same with phpcr-odm. i created http://www.doctrine-project.org/jira/browse/PHPCR-71 to track this further - i think we should not block this pull request by it. having the same behaviour over all 3 mappers is definitively better than having diverging behaviour.

@stof
Doctrine member
stof added a note Jun 12, 2012

not sure.

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 Jun 11, 2012
lib/Doctrine/ODM/PHPCR/Mapping/Driver/YamlDriver.php
@@ -84,7 +91,18 @@ public function loadMetadataForClass($className, ClassMetadata $class)
}
}
if (isset($element['id'])) {
- $mapping = array('fieldName' => $element['id'], 'id' => true);
+ if(is_array($element['id'])) {
+ if(!isset($element['id']['fieldName'])) {
+ throw new MappingException("Missing fieldName property for id field");
+ }
+ else {
@stof
Doctrine member
stof added a note Jun 11, 2012

else should be on the same line than the closing curly brace of the corresponding if

@stof
Doctrine member
stof added a note Jun 11, 2012

btw, you don't need to use else in this place as the if leaves the function because of the exception

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

This pull request passes (merged bd15c45 into dbd116e).

@travisbot

This pull request passes (merged 55bc9fc into 50b4ee1).

@lsmith77 lsmith77 commented on an outdated diff Jun 13, 2012
lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadata.php
@@ -920,6 +924,10 @@ public function mapField(array $mapping)
if ($mapping['type'] === 'int') {
$mapping['type'] = 'long';
}
+
@lsmith77
Doctrine member
lsmith77 added a note Jun 13, 2012

should be an elseif .. ?

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

This pull request fails (merged f30153b into 50b4ee1).

@travisbot

This pull request fails (merged 516f928 into 50b4ee1).

@dbu
Doctrine member
dbu commented Jun 14, 2012

is this failure a bug that is reveiled by the changes to the tests or a bug in one of the code cleanups?

Doctrine\Tests\ODM\PHPCR\Mapping\AnnotationDriverTest::testLoadMappedSuperclassTypeMapping
Undefined property: Doctrine\ODM\PHPCR\Mapping\Annotations\MappedSuperclass::$nodeType

/home/vagrant/builds/doctrine/phpcr-odm/lib/Doctrine/ODM/PHPCR/Mapping/Driver/AnnotationDriver.php:87
/home/vagrant/builds/doctrine/phpcr-odm/tests/Doctrine/Tests/ODM/PHPCR/Mapping/AbstractMappingDriverTest.php:31
/home/vagrant/builds/doctrine/phpcr-odm/tests/Doctrine/Tests/ODM/PHPCR/Mapping/AbstractMappingDriverTest.php:399
/home/vagrant/.phpenv/versions/5.3.13/bin/phpunit:46

apart from that i think we should merge now and do further things in a separate pull request, otherwise it becomes to big.

@stof
Doctrine member
stof commented Jun 14, 2012

from what I see in the code, it is an existing bug which was not spotted by the testsuite previously but is now covered. And it is indeed right: the nodeType property is defined in Document but not in MappedSuperclass`` whereas it will still try to access it for mapped superclasses (other properties will fail too): https://github.com/craigmarvelley/phpcr-odm/blob/516f92848bb7bb38eb74c2aed0562e0fafe8f35e/lib/Doctrine/ODM/PHPCR/Mapping/Driver/AnnotationDriver.php#L58

@craigmarvelley

Sorry, only now seeing the failing test - what's the best approach to take here? Extend Document and MappedSuperclass from an abstract parent, and move those properties there?

@craigmarvelley

I couldn't find any docs on MappedSuperclass so wasn't sure it made sense for that and Document to extend a common parent, so I've followed Doctrine ORM's lead, and like Doctrine\ORM\Mapping\Entity and Doctrine\ORM\Mapping\MappedSuperclass I've kept them apart and just replicated the common properties for now.

@travisbot

This pull request passes (merged 4162a4d into 50b4ee1).

@dbu dbu merged commit 96e876b into doctrine:master Jun 15, 2012
@dbu
Doctrine member
dbu commented Jul 18, 2012

hi @craigmarvelley

bringing this up again, as i started to write the phpcr-odm documentation and stumbled over the mapped super class. as far as i can see, this is just a cludge for the inflexibilty of an ORM data store. this is how i currently document the feature:

https://github.com/doctrine/phpcr-odm-documentation/blob/master/en/reference/inheritance-mapping.rst

i think we can just ditch the explicit mapped superclass thing, it makes no sense for phpcr-odm. if somebody explicitly never wants to store the super class, he can just make it an abstract class in the php sense. does that make sense?

/cc @lsmith77

@lsmith77
Doctrine member

we need to discuss this with the other ODM devs @beberlei, @jmikola, @jwage, @odino

@craigmarvelley

Handling it with abstract classes makes sense to me. Is the intention of the MappedSuperclass annotation to just let you specify a single class in the inheritance chain from which additional annotations will be parsed, rather than recursing through all ancestors? Is that the functionality that we're after here?

I started implementing some more unit tests to achieve mapping driver parity here: https://github.com/craigmarvelley/phpcr-odm/tree/more-mapping-code-coverage, can roll any other changes into that work if necessary. Also just started paternity leave though, so you may have to bear with me time-wise :)

@beberlei
Doctrine member

@dbu how on this abstract class is the person going to define mapping information in annotatinos? That is what the Mapped superclass is for. If no annotation document or MSC is found, no parsing of metadata is done.

@dbu
Doctrine member
dbu commented Jul 19, 2012

@beberlei so the xml/yml mapping drivers will not look at the class and check if its parent class has mappings too?
as we do not have the limitations on extending document classes with the odm, we need something more flexible where the parent class does not need to know it is going to be extended... in the orm the superclass thing is for the parent class, right? we would rather need to have something like (yml) inheritMappings: My\Super\Class to make the loader get the metadata from that parent and merge them in.

  • note to self: we should load the meta data through the loader, so the parent class can have the mappings defined in a different way than the extending class.
@dbu
Doctrine member
dbu commented Jul 19, 2012

aaah. i did not really read. are you saying that i use this to make doctrine understand that my class which is just extending a mapped class without adding any mappings of its own should get the mappings of the super class when i use xml/yml mapping?

@beberlei
Doctrine member

you are thinking about this wrong. This has nothing to do with xm/yml.

Mapped superclasses are abstract base classes, which can be reused in any persistent document. These persistent document classes inherit all mapped properties of the mapped superclass. This is not a limitation of the persistence storatge, but of single inheritance languages such as PHP.

@dbu
Doctrine member
dbu commented Jul 19, 2012

ok, so it is about mapping inheritance not following class inheritance? what happens with class inheritance, is it usually ignored unless you explicitly map the class as superclass? or is the MSC just overwriting mapping inheritance following the class inheritance that would normally happen?

@dbu
Doctrine member
dbu commented Jul 23, 2012

i created http://www.doctrine-project.org/jira/browse/PHPCR-75 to further track this.

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.