Permalink
Browse files

[DDC-1604] Have ORM Proxy implement new \Doctrine\Common\Persistence\…

…Proxy

* Adjust ProxyFactory to generate proxies according to new naming schema.
* Change proxy naming and file-name generation to be a bit more consistent than previous approach.

[DDC-1598] Additional regexp to check for simple ID methods to make it even more safe.
  • Loading branch information...
beberlei committed Jan 16, 2012
1 parent 27451a5 commit a029b284234065a03dab4241b29f0325975eeac6
@@ -1,7 +1,5 @@
<?php
/*
- * $Id$
- *
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
@@ -21,10 +19,12 @@
namespace Doctrine\ORM\Proxy;
+use Doctrine\Common\Persistence\Proxy as BaseProxy;
+
/**
* Interface for proxy classes.
*
* @author Roman Borschel <roman@code-factory.org>
* @since 2.0
*/
-interface Proxy {}
+interface Proxy extends BaseProxy {}
@@ -21,7 +21,8 @@
use Doctrine\ORM\EntityManager,
Doctrine\ORM\Mapping\ClassMetadata,
- Doctrine\ORM\Mapping\AssociationMapping;
+ Doctrine\ORM\Mapping\AssociationMapping,
+ Doctrine\Common\Util\ClassUtils;
/**
* This factory is used to create proxy objects for entities at runtime.
@@ -41,6 +42,14 @@ class ProxyFactory
/** The directory that contains all proxy classes. */
private $_proxyDir;
+ /**
+ * Used to match very simple id methods that don't need
+ * to be proxied since the identifier is known.
+ *
+ * @var string
+ */
+ const PATTERN_MATCH_ID_METHOD = '((public\s)?(function\s{1,}%s\s?\(\)\s{1,})\s{0,}{\s{0,}return\s{0,}\$this->%s;\s{0,}})i';
+
/**
* Initializes a new instance of the <tt>ProxyFactory</tt> class that is
* connected to the given <tt>EntityManager</tt>.
@@ -74,13 +83,12 @@ public function __construct(EntityManager $em, $proxyDir, $proxyNs, $autoGenerat
*/
public function getProxy($className, $identifier)
{
- $proxyClassName = str_replace('\\', '', $className) . 'Proxy';
- $fqn = $this->_proxyNamespace . '\\' . $proxyClassName;
+ $fqn = ClassUtils::generateProxyClassName($className, $this->_proxyNamespace);
if (! class_exists($fqn, false)) {
- $fileName = $this->_proxyDir . DIRECTORY_SEPARATOR . $proxyClassName . '.php';
+ $fileName = $this->getProxyFileName($className);
if ($this->_autoGenerate) {
- $this->_generateProxyClass($this->_em->getClassMetadata($className), $proxyClassName, $fileName, self::$_proxyClassTemplate);
+ $this->_generateProxyClass($this->_em->getClassMetadata($className), $fileName, self::$_proxyClassTemplate);
}
require $fileName;
}
@@ -94,6 +102,17 @@ public function getProxy($className, $identifier)
return new $fqn($entityPersister, $identifier);
}
+ /**
+ * Generate the Proxy file name
+ *
+ * @param string $className
+ * @return string
+ */
+ private function getProxyFileName($className)
+ {

This comment has been minimized.

Show comment Hide comment
@stof

stof Jan 16, 2012

Member

shouldn't this use the MARKER constant ?

@stof

stof Jan 16, 2012

Member

shouldn't this use the MARKER constant ?

+ return $this->_proxyDir . DIRECTORY_SEPARATOR . '__CG__' . str_replace('\\', '', $className) . '.php';
+ }
+
/**
* Generates proxy classes for all given classes.
*
@@ -112,21 +131,19 @@ public function generateProxyClasses(array $classes, $toDir = null)
continue;
}
- $proxyClassName = str_replace('\\', '', $class->name) . 'Proxy';
- $proxyFileName = $proxyDir . $proxyClassName . '.php';

This comment has been minimized.

Show comment Hide comment
@gedrox

gedrox Feb 29, 2012

Contributor

After this change the method argument $toDir does not affect anything.

@gedrox

gedrox Feb 29, 2012

Contributor

After this change the method argument $toDir does not affect anything.

- $this->_generateProxyClass($class, $proxyClassName, $proxyFileName, self::$_proxyClassTemplate);
+ $proxyFileName = $this->getProxyFileName($class->name);
+ $this->_generateProxyClass($class, $proxyFileName, self::$_proxyClassTemplate);
}
}
/**
* Generates a proxy class file.
*
* @param $class
- * @param $originalClassName
* @param $proxyClassName
* @param $file The path of the file to write to.
*/
- private function _generateProxyClass($class, $proxyClassName, $fileName, $file)
+ private function _generateProxyClass($class, $fileName, $file)
{
$methods = $this->_generateMethods($class);
$sleepImpl = $this->_generateSleep($class);
@@ -138,16 +155,19 @@ private function _generateProxyClass($class, $proxyClassName, $fileName, $file)
'<methods>', '<sleepImpl>', '<cloneImpl>'
);
- if(substr($class->name, 0, 1) == "\\") {
- $className = substr($class->name, 1);
- } else {
- $className = $class->name;
- }
+ $className = ltrim($class->name, '\\');
+ $proxyClassName = ClassUtils::generateProxyClassName($class->name, $this->_proxyNamespace);
+ $parts = explode('\\', strrev($proxyClassName), 2);
+ $proxyClassNamespace = strrev($parts[1]);
+ $proxyClassName = strrev($parts[0]);
$replacements = array(
- $this->_proxyNamespace,
- $proxyClassName, $className,
- $methods, $sleepImpl, $cloneImpl
+ $proxyClassNamespace,
+ $proxyClassName,
+ $className,
+ $methods,
+ $sleepImpl,
+ $cloneImpl
);
$file = str_replace($placeholders, $replacements, $file);
@@ -230,21 +250,41 @@ private function _generateMethods(ClassMetadata $class)
}
/**
+ * Check if the method is a short identifier getter.
+ *
+ * What does this mean? For proxy objects the identifier is already known,
+ * however accessing the getter for this identifier usually triggers the
+ * lazy loading, leading to a query that may not be necessary if only the
+ * ID is interesting for the userland code (for example in views that
+ * generate links to the entity, but do not display anything else).
+ *
* @param ReflectionMethod $method
* @param ClassMetadata $class
* @return bool
*/
private function isShortIdentifierGetter($method, $class)
{
$identifier = lcfirst(substr($method->getName(), 3));
- return (
+ $cheapCheck = (
$method->getNumberOfParameters() == 0 &&
substr($method->getName(), 0, 3) == "get" &&
in_array($identifier, $class->identifier, true) &&
$class->hasField($identifier) &&
(($method->getEndLine() - $method->getStartLine()) <= 4)
&& in_array($class->fieldMappings[$identifier]['type'], array('integer', 'bigint', 'smallint', 'string'))
);
+
+ if ($cheapCheck) {
+ $code = file($class->reflClass->getFileName());
+ $code = trim(implode(" ", array_slice($code, $method->getStartLine() - 1, $method->getEndLine() - $method->getStartLine() + 1)));
+
+ $pattern = sprintf(self::PATTERN_MATCH_ID_METHOD, $method->getName(), $identifier);
+
+ if (preg_match($pattern, $code)) {
+ return true;
+ }
+ }
+ return false;
}
/**
@@ -318,6 +358,12 @@ public function __load()
}
}
+ /** @private */
+ public function __isInitialized()
+ {
+ return $this->__isInitialized__;
+ }
+
<methods>
public function __sleep()
@@ -195,4 +195,28 @@ public function testInitializeProxyOnGettingSomethingOtherThanTheIdentifier()
$this->assertEquals('Doctrine Cookbook', $entity->getName());
$this->assertTrue($entity->__isInitialized__, "Getting something other than the identifier initializes the proxy.");
}
+
+ /**
+ * @group DDC-1604
+ */
+ public function testCommonPersistenceProxy()
+ {
+ $id = $this->createProduct();
+
+ /* @var $entity Doctrine\Tests\Models\ECommerce\ECommerceProduct */
+ $entity = $this->_em->getReference('Doctrine\Tests\Models\ECommerce\ECommerceProduct' , $id);
+ $className = \Doctrine\Common\Util\ClassUtils::getClass($entity);
+
+ $this->assertInstanceOf('Doctrine\Common\Persistence\Proxy', $entity);
+ $this->assertFalse($entity->__isInitialized());
+ $this->assertEquals('Doctrine\Tests\Models\ECommerce\ECommerceProduct', $className);
+
+ $restName = str_replace($this->_em->getConfiguration()->getProxyNamespace(), "", get_class($entity));
+ $restName = substr(get_class($entity), strlen($this->_em->getConfiguration()->getProxyNamespace()) +1);
+ $proxyFileName = $this->_em->getConfiguration()->getProxyDir() . DIRECTORY_SEPARATOR . str_replace("\\", "", $restName) . ".php";
+ $this->assertTrue(file_exists($proxyFileName), "Proxy file name cannot be found generically.");
+
+ $entity->__load();
+ $this->assertTrue($entity->__isInitialized());
+ }
}
@@ -52,7 +52,7 @@ protected function tearDown()
public function testReferenceProxyDelegatesLoadingToThePersister()
{
$identifier = array('id' => 42);
- $proxyClass = 'Proxies\DoctrineTestsModelsECommerceECommerceFeatureProxy';
+ $proxyClass = 'Proxies\__CG__\Doctrine\Tests\Models\ECommerce\ECommerceFeature';
$persister = $this->_getMockPersister();
$this->_uowMock->setEntityPersister('Doctrine\Tests\Models\ECommerce\ECommerceFeature', $persister);
@@ -69,7 +69,7 @@ public function testReferenceProxyDelegatesLoadingToThePersister()
public function testReferenceProxyExecutesLoadingOnlyOnce()
{
$identifier = array('id' => 42);
- $proxyClass = 'Proxies\DoctrineTestsModelsECommerceECommerceFeatureProxy';
+ $proxyClass = 'Proxies\__CG__\Doctrine\Tests\Models\ECommerce\ECommerceFeature';
$persister = $this->_getMockPersister();
$this->_uowMock->setEntityPersister('Doctrine\Tests\Models\ECommerce\ECommerceFeature', $persister);
$proxy = $this->_proxyFactory->getProxy('Doctrine\Tests\Models\ECommerce\ECommerceFeature', $identifier);
@@ -108,7 +108,7 @@ public function testProxyRespectsMethodsWhichReturnValuesByReference() {
public function testCreatesAssociationProxyAsSubclassOfTheOriginalOne()
{
- $proxyClass = 'Proxies\DoctrineTestsModelsECommerceECommerceFeatureProxy';
+ $proxyClass = 'Proxies\__CG__\Doctrine\Tests\Models\ECommerce\ECommerceFeature';
$this->assertTrue(is_subclass_of($proxyClass, 'Doctrine\Tests\Models\ECommerce\ECommerceFeature'));
}
@@ -125,32 +125,32 @@ public function testNonNamespacedProxyGeneration()
require_once dirname(__FILE__)."/fixtures/NonNamespacedProxies.php";
$className = "\DoctrineOrmTestEntity";
- $proxyName = "DoctrineOrmTestEntityProxy";
+ $proxyName = "DoctrineOrmTestEntity";
$classMetadata = new \Doctrine\ORM\Mapping\ClassMetadata($className);
$classMetadata->initializeReflection(new \Doctrine\Common\Persistence\Mapping\RuntimeReflectionService);
$classMetadata->mapField(array('fieldName' => 'id', 'type' => 'integer'));
$classMetadata->setIdentifier(array('id'));
$this->_proxyFactory->generateProxyClasses(array($classMetadata));
- $classCode = file_get_contents(dirname(__FILE__)."/generated/".$proxyName.".php");
+ $classCode = file_get_contents(dirname(__FILE__)."/generated/__CG__".$proxyName.".php");
- $this->assertNotContains("class DoctrineOrmTestEntityProxy extends \\\\DoctrineOrmTestEntity", $classCode);
- $this->assertContains("class DoctrineOrmTestEntityProxy extends \\DoctrineOrmTestEntity", $classCode);
+ $this->assertNotContains("class DoctrineOrmTestEntity extends \\\\DoctrineOrmTestEntity", $classCode);
+ $this->assertContains("class DoctrineOrmTestEntity extends \\DoctrineOrmTestEntity", $classCode);
}
public function testClassWithSleepProxyGeneration()
{
$className = "\Doctrine\Tests\ORM\Proxy\SleepClass";
- $proxyName = "DoctrineTestsORMProxySleepClassProxy";
+ $proxyName = "DoctrineTestsORMProxySleepClass";
$classMetadata = new \Doctrine\ORM\Mapping\ClassMetadata($className);
$classMetadata->initializeReflection(new \Doctrine\Common\Persistence\Mapping\RuntimeReflectionService);
$classMetadata->mapField(array('fieldName' => 'id', 'type' => 'integer'));
$classMetadata->setIdentifier(array('id'));
$this->_proxyFactory->generateProxyClasses(array($classMetadata));
- $classCode = file_get_contents(dirname(__FILE__)."/generated/".$proxyName.".php");
+ $classCode = file_get_contents(dirname(__FILE__)."/generated/__CG__".$proxyName.".php");
$this->assertEquals(1, substr_count($classCode, 'function __sleep'));
}

6 comments on commit a029b28

@stloyd

This comment has been minimized.

Show comment Hide comment
@stloyd

stloyd Jan 17, 2012

Contributor

@beberlei Please fix also DoctrineBundle as it's now unusable.

Contributor

stloyd replied Jan 17, 2012

@beberlei Please fix also DoctrineBundle as it's now unusable.

@beberlei

This comment has been minimized.

Show comment Hide comment
@beberlei

beberlei Jan 17, 2012

Owner

@stloyd oh, i was pretty sure that the implementation was backwards compatible. I will see what has to be done.

Owner

beberlei replied Jan 17, 2012

@stloyd oh, i was pretty sure that the implementation was backwards compatible. I will see what has to be done.

@beberlei

This comment has been minimized.

Show comment Hide comment
@beberlei

beberlei Jan 17, 2012

Owner

@stloyd what does "unusable" constitute for you? :)

Owner

beberlei replied Jan 17, 2012

@stloyd what does "unusable" constitute for you? :)

@stloyd

This comment has been minimized.

Show comment Hide comment
@stloyd

stloyd Jan 17, 2012

Contributor

I cannot load proxy files because DoctrineBundle looks for old named proxy files. Here is an error:

The proxy file "/var/www/vhosts/myhost/app/cache/dev/doctrine/orm/Proxies/__CG__\Buu\FooBundle\Entity\Category.php" does not exist. If you still have objects serialized in the session, you need to clear the session manually. 
Contributor

stloyd replied Jan 17, 2012

I cannot load proxy files because DoctrineBundle looks for old named proxy files. Here is an error:

The proxy file "/var/www/vhosts/myhost/app/cache/dev/doctrine/orm/Proxies/__CG__\Buu\FooBundle\Entity\Category.php" does not exist. If you still have objects serialized in the session, you need to clear the session manually. 
@beberlei

This comment has been minimized.

Show comment Hide comment
@beberlei

beberlei Jan 17, 2012

Owner

Ah yes, this is missing. I suppose the proxy file is named __CG__BuuFooBundleEntityCategory.php

Owner

beberlei replied Jan 17, 2012

Ah yes, this is missing. I suppose the proxy file is named __CG__BuuFooBundleEntityCategory.php

@stloyd

This comment has been minimized.

Show comment Hide comment
@stloyd

stloyd Jan 17, 2012

Contributor

Just simple fix commited: doctrine/DoctrineBundle#9

Contributor

stloyd replied Jan 17, 2012

Just simple fix commited: doctrine/DoctrineBundle#9

Please sign in to comment.