Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mapping test improvements #144

Merged
merged 37 commits into from Jun 15, 2012
Merged

Mapping test improvements #144

Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
ba511e6
Moved utility method to abstract test class for use in other driver t…
Jun 9, 2012
b92b6b2
Removed utility class
Jun 9, 2012
a197b02
Added XmlDriverTest
Jun 9, 2012
0120fd5
Made testLoadMetadataForNonDocumentThrowsException and testGetAllClas…
Jun 9, 2012
97c90b6
Maintain mapping exception data when rethrowing exception
Jun 9, 2012
fe8050a
Added CmsAddress XML mapping file
Jun 9, 2012
1b0ed9c
Made testGetAllClassNamesReturnsAlreadyLoadedClassesIfAppropriate an …
Jun 9, 2012
84c4c14
Made testGetAllClassNamesReturnsOnlyTheAppropriateClasses an abstract…
Jun 9, 2012
f1f6ec7
Relocated method for visibility
Jun 9, 2012
c42d5d0
Made loadDriverForCMSDocuments an abstract method in the base class
Jun 9, 2012
3e46d76
Added YamlMappingTest which extends the AbstractMappingDriverTest class
Jun 9, 2012
5ea8cd3
Use a specific class for the testing of property methods. Fixed bug w…
Jun 9, 2012
381ce08
Use 'field' rather than 'property' to make tests less ambiguous
Jun 9, 2012
514205d
Ensure types are correct, and unsupported field types map to their al…
Jun 9, 2012
e06ef56
Added tests for Nodename mapping to abstract driver class
Jun 9, 2012
766ef00
Added tests for ParentDocument mapping to abstract driver class
Jun 9, 2012
40aae5a
Removed redundant files
Jun 9, 2012
3bbd585
Refactored abstract test class to remove repitition
Jun 9, 2012
82e6dc8
Added tests for Child mapping to abstract driver class
Jun 9, 2012
fec02f4
Added tests for Children mapping to abstract driver class
Jun 9, 2012
41039a4
Throw an exception if the fieldName of a children field is blank
Jun 9, 2012
fb0cce4
Removed redundant tests
Jun 9, 2012
b6bf172
Removed unused file
Jun 9, 2012
5e9c90c
Singularized Models directory for consistency
Jun 9, 2012
35b9990
Added tests for specifying a Repository strategy to abstract driver c…
Jun 9, 2012
1c5bbd9
Removed unnecessary isset()
Jun 12, 2012
9d594c1
Maintain exception stack
Jun 12, 2012
6207513
CS fixes, removed redundant else
Jun 12, 2012
b1adc86
Explicitly use default YAML properties
Jun 12, 2012
bd15c45
CS fixes
Jun 12, 2012
55bc9fc
Merge remote-tracking branch 'original/master' into mapping-test-impr…
craigmarvelley Jun 13, 2012
d747570
Added tests for setting versionable annotation metadata
craigmarvelley Jun 13, 2012
1a4e99b
Added tests for setting NodeType and MappedSuperclass annotation meta…
craigmarvelley Jun 13, 2012
f30153b
Added tests for setting Node annotation metadata
craigmarvelley Jun 13, 2012
516f928
Use an elseif when checking field mapping type
craigmarvelley Jun 13, 2012
be92b27
XML and YAML drivers load ReferenceMany metadata mappings
craigmarvelley Jun 13, 2012
4162a4d
Added missing properties to MappedSuperclass annotation
craigmarvelley Jun 14, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions lib/Doctrine/ODM/PHPCR/Mapping/Annotations/DoctrineAnnotations.php
View file
Open in desktop
Expand Up @@ -168,7 +168,7 @@ final class Boolean extends TranslatableProperty
*/
final class Name extends TranslatableProperty
{
public $type = 'string';
public $type = 'name';
}
/**
* String that is an absolute or relative path in the repository
Expand All @@ -177,7 +177,7 @@ final class Name extends TranslatableProperty
*/
final class Path extends TranslatableProperty
{
public $type = 'string';
public $type = 'path';
}
/**
* String that is validated to be an URI
Expand All @@ -186,7 +186,7 @@ final class Path extends TranslatableProperty
*/
final class Uri extends TranslatableProperty
{
public $type = 'string';
public $type = 'uri';
}
/**
* Large numbers bcmath compatible strings
Expand All @@ -195,7 +195,7 @@ final class Uri extends TranslatableProperty
*/
final class Decimal extends TranslatableProperty
{
public $type = 'string';
public $type = 'decimal';
}

/**
Expand Down
10 changes: 9 additions & 1 deletion lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadata.php
View file
Open in desktop
Expand Up @@ -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'])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

throw new MappingException("Mapping a property requires to specify the fieldName.");
}

if (!is_string($mapping['fieldName'])) {
throw new MappingException("fieldName must be of type string.");
}

if ($isField && !isset($mapping['name'])) {
$mapping['name'] = $mapping['fieldName'];
Expand Down Expand Up @@ -919,6 +923,10 @@ public function mapField(array $mapping)
if ($mapping['type'] === 'int') {
$mapping['type'] = 'long';
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be an elseif .. ?

if ($mapping['type'] === 'float') {
$mapping['type'] = 'double';
}

// Add the field to the list of translatable fields
if (isset($mapping['translated']) && $mapping['translated']) {
Expand Down
9 changes: 8 additions & 1 deletion lib/Doctrine/ODM/PHPCR/Mapping/Driver/XmlDriver.php
View file
Open in desktop
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing a space after catch

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Convert Exception type for consistency with other drivers
throw new MappingException($e->getMessage(), $e->getCode(), $e->getPrevious());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure.

}

if (!$xmlRoot) {
return;
}
Expand Down
22 changes: 20 additions & 2 deletions lib/Doctrine/ODM/PHPCR/Mapping/Driver/YamlDriver.php
View file
Open in desktop
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above notes

// Convert Exception type for consistency with other drivers
throw new MappingException($e->getMessage(), $e->getCode(), $e->getPrevious());
}

if (!$element) {
return;
}
Expand Down Expand Up @@ -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'])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if(!isset($element['id']['fieldName'])) {
throw new MappingException("Missing fieldName property for id field");
}
else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$fieldName = $element['id']['fieldName'];
}
}
else {
$fieldName = $element['id'];
}
$mapping = array('fieldName' => $fieldName, 'id' => true);
if (isset($element['id']['generator']['strategy'])) {
$mapping['strategy'] = $element['id']['generator']['strategy'];
}
Expand Down
26 changes: 24 additions & 2 deletions tests/Doctrine/Tests/ODM/PHPCR/Functional/PropertyTest.php
View file
Open in desktop
Expand Up @@ -3,6 +3,7 @@
namespace Doctrine\Tests\ODM\PHPCR\Functional;

use Doctrine\ODM\PHPCR\Mapping\Annotations as PHPCRODM;
use PHPCR\PropertyType;

/**
* @group functional
Expand Down Expand Up @@ -46,24 +47,45 @@ public function testPropertyname()
$doc->date = $date;
$doc->boolean = true;
$doc->name = 'aname';
$doc->path = '../';
$doc->path = '../a';
$doc->uri = 'http://cmf.symfony.com:8080/about.html#there';

$this->dm->persist($doc);
$this->dm->flush();
$this->dm->clear();

$this->assertTrue($this->node->getNode('p')->hasProperty('string'));
$this->assertEquals(PropertyType::STRING, $this->node->getNode('p')->getProperty('string')->getType());

$this->assertTrue($this->node->getNode('p')->hasProperty('long'));
$this->assertEquals(PropertyType::LONG, $this->node->getNode('p')->getProperty('long')->getType());

$this->assertTrue($this->node->getNode('p')->hasProperty('int'));
$this->assertEquals(PropertyType::LONG, $this->node->getNode('p')->getProperty('int')->getType());

$this->assertTrue($this->node->getNode('p')->hasProperty('decimal'));
$this->assertEquals(PropertyType::DECIMAL, $this->node->getNode('p')->getProperty('decimal')->getType());

$this->assertTrue($this->node->getNode('p')->hasProperty('double'));
$this->assertEquals(PropertyType::DOUBLE, $this->node->getNode('p')->getProperty('double')->getType());

$this->assertTrue($this->node->getNode('p')->hasProperty('float'));
$this->assertEquals(PropertyType::DOUBLE, $this->node->getNode('p')->getProperty('float')->getType());

$this->assertTrue($this->node->getNode('p')->hasProperty('date'));
$this->assertEquals(PropertyType::DATE, $this->node->getNode('p')->getProperty('date')->getType());

$this->assertTrue($this->node->getNode('p')->hasProperty('boolean'));
$this->assertEquals(PropertyType::BOOLEAN, $this->node->getNode('p')->getProperty('boolean')->getType());

$this->assertTrue($this->node->getNode('p')->hasProperty('name'));
$this->assertEquals(PropertyType::NAME, $this->node->getNode('p')->getProperty('name')->getType());

$this->assertTrue($this->node->getNode('p')->hasProperty('path'));
$this->assertEquals(PropertyType::PATH, $this->node->getNode('p')->getProperty('path')->getType());

$this->assertTrue($this->node->getNode('p')->hasProperty('uri'));
$this->assertEquals(PropertyType::URI, $this->node->getNode('p')->getProperty('uri')->getType());

$doc = $this->dm->find($this->type, '/functional/p');
$this->assertNotNull($doc->string);
Expand All @@ -85,7 +107,7 @@ public function testPropertyname()
$this->assertNotNull($doc->name);
$this->assertEquals('aname', $doc->name);
$this->assertNotNull($doc->path);
$this->assertEquals('../', $doc->path);
$this->assertEquals('../a', $doc->path);
$this->assertNotNull($doc->uri);
$this->assertEquals('http://cmf.symfony.com:8080/about.html#there', $doc->uri);
}
Expand Down