[DCOM-96] Refactor ProxyGenerator, introduce AbstractProxyFactory #247

Merged
merged 4 commits into from Jan 26, 2013

Projects

None yet

5 participants

@beberlei
Member

This PR does two things:

  • ProxyGenerator is refactored to use lazy templating.
  • AbstractProxyFactory is introduced to move more logic into Common
@doctrinebot
Collaborator

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DDC-1938

@Ocramius Ocramius commented on an outdated diff Jan 25, 2013
lib/Doctrine/Common/Proxy/ProxyGenerator.php
{
- return ClassUtils::generateProxyClassName($originalClassName, $this->proxyNamespace);
+ $placeholders = array();
+
+ preg_match_all('(<([a-zA-Z]+)>)', $this->proxyClassTemplate, $placeholderMatches);
+ $placeholderMatches = array_combine($placeholderMatches[0], $placeholderMatches[1]);
+
+ foreach ($placeholderMatches as $placeholder => $name) {
+ $placeholders[$placeholder] = isset($this->placeholders[$name])
+ ? $this->placeholders[$name]
@Ocramius
Ocramius Jan 25, 2013 Doctrine member

Don't forget that I need to have a closure here :\

@guilhermeblanco guilhermeblanco commented on an outdated diff Jan 25, 2013
lib/Doctrine/Common/Proxy/AbstractProxyFactory.php
+ * directory configured on the Configuration of the EntityManager used
+ * by this factory is used.
+ * @return int Number of generated proxies.
+ */
+ public function generateProxyClasses(array $classes, $proxyDir = null)
+ {
+ $generated = 0;
+
+ foreach ($classes as $class) {
+ /* @var $class \Doctrine\ORM\Mapping\ClassMetadataInfo */
+ if ($this->skipClass($class)) {
+ continue;
+ }
+
+ $proxyFileName = $generator->getProxyFileName($class->getName(), $proxyDir);
+ $generator->generateProxyClass($class, $proxyFileName);
@guilhermeblanco
guilhermeblanco Jan 25, 2013 Doctrine member

Missing line break here.
They're two different logical blocks. First is assignment, second is method call

@guilhermeblanco guilhermeblanco commented on an outdated diff Jan 25, 2013
lib/Doctrine/Common/Proxy/AbstractProxyFactory.php
+ return $proxy;
+ }
+
+ /**
+ * Get a proxy definition for the given class name.
+ *
+ * @return ProxyDefinition
+ */
+ private function getProxyDefinition($className)
+ {
+ if ( ! isset($this->definitions[$className])) {
+ $classMetadata = $this->objectManager->getClassMetadata($className);
+ $className = $classMetadata->getName(); // make sure case-sensitivy is correct
+
+ $this->definitions[$className] = $this->createProxyDefinition($className);
+ $proxyClassName = $this->definitions[$className]->proxyClassName;
@guilhermeblanco
guilhermeblanco Jan 25, 2013 Doctrine member

Missing line break. They're two different logical blocks.
First is class property assignment, second is local variable assignment

@guilhermeblanco guilhermeblanco commented on an outdated diff Jan 25, 2013
lib/Doctrine/Common/Proxy/ProxyGenerator.php
{
- return ClassUtils::generateProxyClassName($originalClassName, $this->proxyNamespace);
+ $placeholders = array();
+
+ preg_match_all('(<([a-zA-Z]+)>)', $this->proxyClassTemplate, $placeholderMatches);
+ $placeholderMatches = array_combine($placeholderMatches[0], $placeholderMatches[1]);
@guilhermeblanco
guilhermeblanco Jan 25, 2013 Doctrine member

Missing line break. Function call on first, local variable assignment on other.

@guilhermeblanco guilhermeblanco and 1 other commented on an outdated diff Jan 25, 2013
lib/Doctrine/Common/Proxy/ProxyGenerator.php
{
- return ClassUtils::generateProxyClassName($originalClassName, $this->proxyNamespace);
+ $placeholders = array();
+
+ preg_match_all('(<([a-zA-Z]+)>)', $this->proxyClassTemplate, $placeholderMatches);
+ $placeholderMatches = array_combine($placeholderMatches[0], $placeholderMatches[1]);
+
+ foreach ($placeholderMatches as $placeholder => $name) {
+ $placeholders[$placeholder] = isset($this->placeholders[$name])
+ ? $this->placeholders[$name]
+ : call_user_func(array($this, "generate" . $name), $class);
+ }
+
+ $fileName = $fileName ?: $this->getProxyFileName($class->getName());
+ $file = $file ? $file : $this->proxyClassTemplate;
@guilhermeblanco
guilhermeblanco Jan 25, 2013 Doctrine member

Please align = signs.

@guilhermeblanco
guilhermeblanco Jan 25, 2013 Doctrine member

Also, you could use a short ternary operator here. =)

@Ocramius
Ocramius Jan 25, 2013 Doctrine member

Huh? Why not? O_o (this one is new)

@guilhermeblanco
guilhermeblanco Jan 25, 2013 Doctrine member
$file = $file ?: $this->proxyClassTemplate;
@guilhermeblanco guilhermeblanco commented on an outdated diff Jan 25, 2013
lib/Doctrine/Common/Proxy/ProxyGenerator.php
+ }
+
+ $fileName = $fileName ?: $this->getProxyFileName($class->getName());
+ $file = $file ? $file : $this->proxyClassTemplate;
+ $file = strtr($file, $placeholders);
+
+ $parentDirectory = dirname($fileName);
+
+ if ( ! is_dir($parentDirectory) && (false === @mkdir($parentDirectory, 0775, true))) {
+ throw UnexpectedValueException::proxyDirectoryNotWritable();
+ } elseif ( ! is_writable($parentDirectory)) {
+ throw UnexpectedValueException::proxyDirectoryNotWritable();
+ }
+
+ $tmpFileName = $fileName . '.' . uniqid('', true);
+ file_put_contents($tmpFileName, $file);
@guilhermeblanco
guilhermeblanco Jan 25, 2013 Doctrine member

Missing line break before this line

@stof stof commented on an outdated diff Jan 25, 2013
lib/Doctrine/Common/Proxy/AbstractProxyFactory.php
+ * @param string $proxyDir The target directory of the proxy classes. If not specified, the
+ * directory configured on the Configuration of the EntityManager used
+ * by this factory is used.
+ * @return int Number of generated proxies.
+ */
+ public function generateProxyClasses(array $classes, $proxyDir = null)
+ {
+ $generated = 0;
+
+ foreach ($classes as $class) {
+ /* @var $class \Doctrine\ORM\Mapping\ClassMetadataInfo */
+ if ($this->skipClass($class)) {
+ continue;
+ }
+
+ $proxyFileName = $generator->getProxyFileName($class->getName(), $proxyDir);
@stof
stof Jan 25, 2013 Doctrine member

undefined variable $generator

@beberlei
Member

This still needs tests

@stof stof commented on an outdated diff Jan 25, 2013
lib/Doctrine/Common/Proxy/ProxyGenerator.php
+ foreach ($placeholderMatches as $placeholder => $name) {
+ $placeholders[$placeholder] = isset($this->placeholders[$name])
+ ? $this->placeholders[$name]
+ : array($this, "generate" . $name);
+ }
+
+ $placeholders = array_map(function ($value) use ($class) {
+ if (is_callable($value)) {
+ return call_user_func($value, $class);
+ }
+
+ return $value;
+ }, $placeholders);
+
+ $fileName = $fileName ?: $this->getProxyFileName($class->getName());
+ $proxyCode = strtr($this->proxyClassTemplate, $placeholders);
@stof
stof Jan 25, 2013 Doctrine member

should be if

@beberlei
Member

Code is final now.

@ocramius Can you give your opinion? I can merge afterwards.

@stof stof commented on an outdated diff Jan 26, 2013
lib/Doctrine/Common/Proxy/AbstractProxyFactory.php
+ /**
+ * Generates proxy classes for all given classes.
+ *
+ * @param \Doctrine\Common\Persistence\Mapping\ClassMetadata[] $classes The classes (ClassMetadata instances)
+ * for which to generate proxies.
+ * @param string $proxyDir The target directory of the proxy classes. If not specified, the
+ * directory configured on the Configuration of the EntityManager used
+ * by this factory is used.
+ * @return int Number of generated proxies.
+ */
+ public function generateProxyClasses(array $classes, $proxyDir = null)
+ {
+ $generated = 0;
+
+ foreach ($classes as $class) {
+ /* @var $class \Doctrine\ORM\Mapping\ClassMetadataInfo */
@stof
stof Jan 26, 2013 Doctrine member

This is wrong. It should be the common interface here (and btw, the param phpdoc should be enough)

@stof stof commented on an outdated diff Jan 26, 2013
lib/Doctrine/Common/Proxy/ProxyGenerator.php
+ $placeholders = array_map(function ($value) use ($class) {
+ if (is_callable($value)) {
+ return call_user_func($value, $class);
+ }
+
+ return $value;
+ }, $placeholders);
+
+ $fileName = $fileName ?: $this->getProxyFileName($class->getName());
+ $proxyCode = strtr($this->proxyClassTemplate, $placeholders);
+
+ $parentDirectory = dirname($fileName);
+
+ if ( ! is_dir($parentDirectory) && (false === @mkdir($parentDirectory, 0775, true))) {
+ throw UnexpectedValueException::proxyDirectoryNotWritable();
+ } elseif ( ! is_writable($parentDirectory)) {
@stof
stof Jan 26, 2013 Doctrine member

it should simply be if here

@Ocramius Ocramius commented on an outdated diff Jan 26, 2013
lib/Doctrine/Common/Proxy/AbstractProxyFactory.php
+ * @var \Doctrine\Common\Proxy\ProxyGenerator the proxy generator responsible for creating the proxy classes/files.
+ */
+ private $proxyGenerator;
+
+ /**
+ * @var bool Whether to automatically (re)generate proxy classes.
+ */
+ private $autoGenerate;
+
+ /**
+ * @var array
+ */
+ private $definitions = array();
+
+ /**
+ * @param ProxyGenerator $proxyGenerator
@Ocramius
Ocramius Jan 26, 2013 Doctrine member

Not in sync with parameters. Also, as guilherme said, let's use FQCN

@Ocramius Ocramius commented on an outdated diff Jan 26, 2013
lib/Doctrine/Common/Proxy/AbstractProxyFactory.php
+ * @param ProxyGenerator $proxyGenerator
+ * @param ObjectManager $objectManager
+ */
+ public function __construct(ProxyGenerator $proxyGenerator, ObjectManager $objectManager, $autoGenerate)
+ {
+ $this->proxyGenerator = $proxyGenerator;
+ $this->objectManager = $objectManager;
+ $this->autoGenerate = $autoGenerate;
+ }
+
+ /**
+ * Gets a reference proxy instance for the entity of the given type and identified by
+ * the given identifier.
+ *
+ * @param string $className
+ * @param mixed $identifier
@Ocramius
Ocramius Jan 26, 2013 Doctrine member

Should be typehinted array I suppose, that's also what you get from the class metadata

@Ocramius Ocramius commented on an outdated diff Jan 26, 2013
lib/Doctrine/Common/Proxy/AbstractProxyFactory.php
+ $this->objectManager = $objectManager;
+ $this->autoGenerate = $autoGenerate;
+ }
+
+ /**
+ * Gets a reference proxy instance for the entity of the given type and identified by
+ * the given identifier.
+ *
+ * @param string $className
+ * @param mixed $identifier
+ *
+ * @return \Doctrine\Common\Proxy\Proxy
+ */
+ public function getProxy($className, $identifier)
+ {
+ $definition = $this->getProxyDefinition($className);
@Ocramius
Ocramius Jan 26, 2013 Doctrine member

Can you avoid the method call here and move the isset from getProxyDefinition to this place?

@Ocramius Ocramius commented on an outdated diff Jan 26, 2013
lib/Doctrine/Common/Proxy/AbstractProxyFactory.php
+ /**
+ * Generates proxy classes for all given classes.
+ *
+ * @param \Doctrine\Common\Persistence\Mapping\ClassMetadata[] $classes The classes (ClassMetadata instances)
+ * for which to generate proxies.
+ * @param string $proxyDir The target directory of the proxy classes. If not specified, the
+ * directory configured on the Configuration of the EntityManager used
+ * by this factory is used.
+ * @return int Number of generated proxies.
+ */
+ public function generateProxyClasses(array $classes, $proxyDir = null)
+ {
+ $generated = 0;
+
+ foreach ($classes as $class) {
+ /* @var $class \Doctrine\Common\Persistence\Mapping\ClassMetadata */
@Ocramius
Ocramius Jan 26, 2013 Doctrine member

Shouldn't need the inline hint here since you handled it in the method docblock

@Ocramius Ocramius commented on an outdated diff Jan 26, 2013
lib/Doctrine/Common/Proxy/AbstractProxyFactory.php
+ * @var \Doctrine\Common\Persistence\ObjectManager
+ */
+ private $objectManager;
+
+ /**
+ * @var \Doctrine\Common\Proxy\ProxyGenerator the proxy generator responsible for creating the proxy classes/files.
+ */
+ private $proxyGenerator;
+
+ /**
+ * @var bool Whether to automatically (re)generate proxy classes.
+ */
+ private $autoGenerate;
+
+ /**
+ * @var array
@Ocramius
Ocramius Jan 26, 2013 Doctrine member

Typehint \Doctrine\Common\Proxy\ProxyDefinition[]

@Ocramius Ocramius commented on an outdated diff Jan 26, 2013
lib/Doctrine/Common/Proxy/AbstractProxyFactory.php
+ * Reset initialization/cloning logic for an un-initialized proxy
+ *
+ * @param \Doctrine\Common\Proxy\Proxy $proxy
+ *
+ * @return \Doctrine\Common\Proxy\Proxy
+ *
+ * @throws \Doctrine\Common\Proxy\Exception\InvalidArgumentException
+ */
+ public function resetUninitializedProxy(Proxy $proxy)
+ {
+ if ($proxy->__isInitialized()) {
+ throw InvalidArgumentException::unitializedProxyExpected();
+ }
+
+ $className = ClassUtils::getClass($proxy);
+ $definition = $this->getProxyDefinition($className);
@Ocramius
Ocramius Jan 26, 2013 Doctrine member

isset($this->definitions) directly before calling the method

@Ocramius Ocramius commented on an outdated diff Jan 26, 2013
lib/Doctrine/Common/Proxy/AbstractProxyFactory.php
+ $definition = $this->getProxyDefinition($className);
+
+ $proxy->__setInitializer($definition->initializer);
+ $proxy->__setCloner($definition->cloner);
+
+ return $proxy;
+ }
+
+ /**
+ * Get a proxy definition for the given class name.
+ *
+ * @return ProxyDefinition
+ */
+ private function getProxyDefinition($className)
+ {
+ if ( ! isset($this->definitions[$className])) {
@Ocramius
Ocramius Jan 26, 2013 Doctrine member

Do it the other way around:

if (isset($this->definitions[$className])) {
    return $this->definitions[$className];
}

// ...
@Ocramius Ocramius commented on an outdated diff Jan 26, 2013
lib/Doctrine/Common/Proxy/AbstractProxyFactory.php
+
+ /**
+ * @var bool Whether to automatically (re)generate proxy classes.
+ */
+ private $autoGenerate;
+
+ /**
+ * @var array
+ */
+ private $definitions = array();
+
+ /**
+ * @param ProxyGenerator $proxyGenerator
+ * @param ObjectManager $objectManager
+ */
+ public function __construct(ProxyGenerator $proxyGenerator, ObjectManager $objectManager, $autoGenerate)
@Ocramius
Ocramius Jan 26, 2013 Doctrine member

Can we avoid injecting an ObjectManager and use only a MetadataFactory here? This would allow proxy usage even when there is no ObjectManager in place (services and similar stuff)

@Ocramius Ocramius commented on an outdated diff Jan 26, 2013
...e/Common/Proxy/Exception/InvalidArgumentException.php
@@ -67,6 +67,11 @@ public static function proxyNamespaceRequired()
return new self('You must configure a proxy namespace');
}
+ public static function unitializedProxyExpected()
@Ocramius
Ocramius Jan 26, 2013 Doctrine member

Add parameter Proxy $proxy and show its classname in the exception

@Ocramius Ocramius commented on an outdated diff Jan 26, 2013
lib/Doctrine/Common/Proxy/ProxyDefinition.php
+ */
+
+namespace Doctrine\Common\Proxy;
+
+/**
+ * Definition structure how to create a proxy.
+ *
+ * @author Benjamin Eberlei <kontakt@beberlei.de>
+ */
+class ProxyDefinition
+{
+ /**
+ * @var string
+ */
+ public $proxyClassName;
+ /**
@Ocramius
Ocramius Jan 26, 2013 Doctrine member

empty newlines between properties

@Ocramius Ocramius added a commit to Ocramius/common that referenced this pull request Jan 26, 2013
@Ocramius Ocramius Applying fixes as commented on doctrine/common#247 f5c1740
@Ocramius
Member

@beberlei applied all my suggestions in #248 for simplicity

@beberlei beberlei merged commit a5482b3 into master Jan 26, 2013
@stof stof referenced this pull request in doctrine/phpcr-odm Jan 27, 2013
Closed

[DCOM-96] proxy generation #214

@Ocramius Ocramius deleted the ProxyFactory branch May 1, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment