Improve code on loadMetadata() to verify if class exists #274

Merged
merged 4 commits into from May 8, 2013

Projects

None yet

3 participants

@entering
Contributor
entering commented May 7, 2013

Improve code on loadMetadata() to verify if class exists, avoid a later warning when calling class_parents()

Instead throws a \RuntimeException if class doesn't exists.

Before changing code, just after add test:

1) Doctrine\Tests\Common\Persistence\Mapping\ClassMetadataFactoryTest::testGetMetadataForAbsentClass
class_parents(): Class Doctrine\Tests\Common\Persistence\Mapping\AbsentClass does not exist and could not be loaded

/home/www/common/lib/Doctrine/Common/Persistence/Mapping/RuntimeReflectionService.php:38
/home/www/common/lib/Doctrine/Common/Persistence/Mapping/AbstractClassMetadataFactory.php:257
/home/www/common/lib/Doctrine/Common/Persistence/Mapping/AbstractClassMetadataFactory.php:281
/home/www/common/lib/Doctrine/Common/Persistence/Mapping/AbstractClassMetadataFactory.php:212
/home/www/common/tests/Doctrine/Tests/Common/Persistence/Mapping/ClassMetadataFactoryTest.php:44
entering added some commits May 7, 2013
@entering entering Improve code on loadMetadata() to verify if class exists, avoid a lat…
…er warning when calling class_parents()

Instead throws a \RuntimeException if class doesn't exists.
68a40cc
@entering entering Improve code to fulfill coding standards. 9ac67af
@Ocramius Ocramius and 1 other commented on an outdated diff May 7, 2013
.../Persistence/Mapping/AbstractClassMetadataFactory.php
@@ -272,6 +272,12 @@ protected function getParentClasses($name)
*/
protected function loadMetadata($name)
{
+ if ( !class_exists($name) ) {
+ throw new \RuntimeException(
@Ocramius
Ocramius May 7, 2013 Member

Can you use an exception from the common namespace?

@entering
entering May 7, 2013 Contributor

I can find the following exceptions: CommonException, MappingException, InvalidArgumentException, UnexpectedValueException.php. I can see this exception being a RuntimeException (but there is no RuntimeException on common namespace) or an InvalidArgumentException, the name of the class should be a valid class, CommonException looks too generic exception too me (it extends \Exception).

InvalidArgumentException from common namespace sounds good to you?

@Ocramius After you opinion I can change the code and update the phpdoc with @throws notation that I forgot.

@Ocramius
Ocramius May 7, 2013 Member

Looks more like a mapping exception to me - mappings cannot be retrieved here, and the argument is not given by the user itself.

Also, move the message generation to a static method of the exception class:

public static function nonExistingClass($className) 
{
    return new self(sprintf(...));
}
@Ocramius Ocramius commented on an outdated diff May 7, 2013
.../Persistence/Mapping/AbstractClassMetadataFactory.php
@@ -272,6 +272,12 @@ protected function getParentClasses($name)
*/
protected function loadMetadata($name)
{
+ if ( !class_exists($name) ) {
@Ocramius
Ocramius May 7, 2013 Member

CS:

if ( ! class_exists($name)) {

@Ocramius Ocramius commented on an outdated diff May 7, 2013
...mmon/Persistence/Mapping/ClassMetadataFactoryTest.php
@@ -39,6 +39,14 @@ public function testGetMetadataFor()
$this->assertTrue($this->cmf->hasMetadataFor('stdClass'));
}
+ /**
+ * @expectedException \RuntimeException
@Ocramius
Ocramius May 7, 2013 Member

Use setExpectedException please

@entering entering Actions done:
- Change exception to MappingException
- Change test to use setExpectedException instead of @expectedException
- Update phpdoc of loadMetadata and fix CS
b8479cb
@entering
Contributor
entering commented May 8, 2013

@Ocramius I changed my code according your suggestions.

@Ocramius Ocramius commented on an outdated diff May 8, 2013
.../Persistence/Mapping/AbstractClassMetadataFactory.php
@@ -20,7 +20,8 @@
namespace Doctrine\Common\Persistence\Mapping;
use Doctrine\Common\Cache\Cache,
@Ocramius
Ocramius May 8, 2013 Member

PSR-2 compliant would be:

use Doctrine\Common\Cache\Cache;
use Doctrine\Common\Util\ClassUtils;
use Doctrine\Common\Persistence\Mapping\MappingException;

So 1 use statement per line

@Ocramius Ocramius commented on an outdated diff May 8, 2013
.../Persistence/Mapping/AbstractClassMetadataFactory.php
@@ -268,10 +269,16 @@ protected function getParentClasses($name)
*
* @param string $name The name of the class for which the metadata should get loaded.
*
+ * @throws MappingException
@Ocramius
Ocramius May 8, 2013 Member

Use the FQCN here: @throws \Doctrine\Common\Persistence\Mapping\MappingException

@Ocramius Ocramius commented on an outdated diff May 8, 2013
...trine/Common/Persistence/Mapping/MappingException.php
@@ -83,4 +83,13 @@ public static function invalidMappingFile($entityName, $fileName)
{
return new self("Invalid mapping file '$fileName' for class '$entityName'.");
}
+
+ /**
+ * @param string $className
+ * @return MappingException
@Ocramius
Ocramius May 8, 2013 Member

Use the FQCN here: @return \Doctrine\Common\Persistence\Mapping\MappingException

Also, one empty newline required between params and return blocks

@Ocramius
Member
Ocramius commented May 8, 2013

@entering fix the last CS, imo it's 👍 then

@entering
Contributor
entering commented May 8, 2013

@Ocramius fixed the CS. I was trying to follow the already existing code and I checked: http://docs.doctrine-project.org/projects/doctrine1/en/latest/en/manual/coding-standards.html and didn't see any mention to PSR-*, probably that documentation needs an update.

@Ocramius
Member
Ocramius commented May 8, 2013

Thats' Doctrine1 documentation, oldoldold ;)

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

On 8 May 2013 09:58, entering notifications@github.com wrote:

@Ocramius https://github.com/Ocramius fixed the CS. I was trying to
follow the already existing code and I checked:
http://docs.doctrine-project.org/projects/doctrine1/en/latest/en/manual/coding-standards.htmland didn't see any mention to PSR-*, probably that documentation needs an
update.


Reply to this email directly or view it on GitHubhttps://github.com/doctrine/common/pull/274#issuecomment-17591611
.

@entering
Contributor
entering commented May 8, 2013

Google tricks me all the time when searching doctrine documentation, I searched "doctrine 2 coding standard" first link was that and I didn't notice was version 1 related. I found this now in a new search: http://www.doctrine-project.org/jira/browse/DDC-585

@Ocramius Ocramius merged commit 32c54b7 into doctrine:master May 8, 2013

1 check failed

default The Travis CI build could not complete due to an error
Details
@beberlei
Member
beberlei commented May 9, 2013

I had to revert this, because ClassMetadata#loadMetadata can be called, when the classes don't exist.

@beberlei
Member
beberlei commented May 9, 2013

If you want to try a new PR, see 7eea788 for a description of the problem and a possible solution.

@entering
Contributor
entering commented May 9, 2013

@beberlei I can open a new PR, but before I would like if placing the class_exists and throw mapping exception at RuntimeReflectionService getParentClasses(), sounds good to you?

@beberlei
Member
beberlei commented May 9, 2013

@entering yes that works :-)

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