[DCOM-96] proxy generation as of doctrine/common#168 #445

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
4 participants
Owner

Ocramius commented Nov 9, 2012

This PR applies changes required to use the new proxy logic implemented in DCOM-96 at doctrine/common#168

Merging this means bumping requirement of doctrine/common to >=2.4.

@jmikola jmikola commented on an outdated diff Nov 9, 2012

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php
@@ -502,8 +496,9 @@ public function setCustomRepositoryClass($repositoryClassName)
* Dispatches the lifecycle event of the given document to the registered
* lifecycle callbacks and lifecycle listeners.
*
- * @param string $event The lifecycle event.
- * @param Document $document The Document on which the event occurred.
+ * @param string $lifecycleEvent The lifecycle event.
+ * @param object $document The Document on which the event occurred.
@jmikola

jmikola Nov 9, 2012

Owner

"Document" can be lower-case

@jmikola jmikola commented on an outdated diff Nov 9, 2012

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php
@@ -21,7 +21,9 @@
use Doctrine\ODM\MongoDB\Mapping\MappingException;
use Doctrine\ODM\MongoDB\LockException;
-use Doctrine\ODM\MongoDB\Proxy\Proxy;
+
+use InvalidArgumentException;
+use ReflectionClass;
@jmikola

jmikola Nov 9, 2012

Owner

Totally nit-picking (so don't be offended), but for global classes, can we avoid use statements and stick to prefixing them with \ in the doc blocks and code? Otherwise there's some ambiguous cases where we refer to \ReflectionProperty but also ReflectionClass.

@jmikola jmikola commented on an outdated diff Nov 9, 2012

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php
*
- * @return string $identifier
+ * Since MongoDB only allows exactly one identifier field this will always return an array with only one value
@jmikola

jmikola Nov 9, 2012

Owner

Word wrap this and the duplicate comment below at 80 characters.

@jmikola jmikola commented on the diff Nov 9, 2012

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php
}
/**
* Get the document identifier object.
*
* @param string $document
- * @return MongoId $id The MongoID object.
+ *
+ * @return \MongoId $id The MongoID object.
@jmikola

jmikola Nov 9, 2012

Owner

Perhaps it's outside the scope of this PR, but how is getIdentifierObject() used? A MongoId is one possible type for an _id, but it can also be an object, string, number, etc. (anything but a numeric array).

@Ocramius

Ocramius Nov 9, 2012

Owner

I don't really know anything about that... I just removed the proxy part since identifier fields are not lazy loaded ;)

@jmikola jmikola commented on an outdated diff Nov 9, 2012

lib/Doctrine/ODM/MongoDB/Proxy/ProxyFactory.php
$this->proxyDir = $proxyDir;
$this->autoGenerate = $autoGenerate;
$this->proxyNamespace = $proxyNs;
}
/**
- * Gets a reference proxy instance for the document of the given type and identified by
+ * Gets a reference proxy instance for the entity of the given type and identified by
@jmikola

jmikola Nov 9, 2012

Owner

ODM uses "document" in place of "entity" in its documentation.

@jmikola jmikola and 1 other commented on an outdated diff Nov 9, 2012

lib/Doctrine/ODM/MongoDB/Proxy/ProxyFactory.php
- $methodNames = array();
- foreach ($class->reflClass->getMethods() as $method) {
- /* @var $method ReflectionMethod */
- if ($method->isConstructor() || in_array(strtolower($method->getName()), array("__sleep", "__clone")) || isset($methodNames[$method->getName()])) {
- continue;
+ if (null === $documentPersister->load($identifier, $proxy)) {
+ throw \Doctrine\ODM\MongoDB\DocumentNotFoundException::documentNotFound(get_class($proxy), $identifier);
@jmikola

jmikola Nov 9, 2012

Owner

DocumentNotFoundException::documentNotFound() expects $identifier to be castable to a string. If I'm following this closure correctly, it's an array at this point.

Since it may be a simple _id (MongoId or scalar), we could cast that value as a string. Otherwise, it may be an object (associative array), in which case JSON encoding is probably best.

Elsewhere, this function is called using the identifier value, so I think we should pass in $identifer['_id'] here (_id can come from a method call for its field name if we don't want to hard-code it). We should also fix documentNotFound() to properly print identifier values that may be associative arrays -- I've created #446 for that.

@Ocramius

Ocramius Nov 9, 2012

Owner

At this point, the identifier is an array, yeah. I'd apply this conversion in the exception class itself, instead of casting before hops :) JSON is a good solution to catch both, but for now I patched to allow array params.

Owner

jmikola commented Nov 9, 2012

Is the proxy loading stuff a BC break? I'm curious because ODM currently requires <2.5-dev of Common, but the current tag for Common is 2.3 (perhaps I should have required <2.4-dev).

If ODM has this problem (i.e. it's a BC break), then ORM will as well, since it also depends on <2.5-dev.

Owner

Ocramius commented Nov 9, 2012

@jmikola ORM does only explicitly require DBAL, not common. We could enforce common <2.4 for the 2.3 series and >=2.4-dev for the 2.4 series...

Member

stof commented Nov 21, 2012

as it requires bumping the requirement, you should change the composer.json constraint to 2.4.*

Member

stof commented Nov 21, 2012

@jmikola Common is BC (or at least should), so using Common 2.4 (with the new proxy generation) with the current ODM proxies works (it simply does not use the new code).

Owner

Ocramius commented Nov 21, 2012

@stof still not sure about what @guilhermeblanco decides, and afaik, 2.4 is behind the corner and I need him to review it first :)

@guilhermeblanco guilhermeblanco commented on the diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php
@@ -236,7 +235,7 @@ class ClassMetadataInfo implements \Doctrine\Common\Persistence\Mapping\ClassMet
/**
* The ReflectionProperty instances of the mapped class.
*
- * @var array
+ * @var \ReflectionProperty[]
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Don't we use the phpDoc2 syntax? It should be \ReflectionProperty<object>, no?

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php
@@ -567,7 +562,8 @@ public function setLifecycleCallbacks(array $callbacks)
* Sets the discriminator field name.
*
* @param string $discriminatorField
- * @see getDiscriminatorField()
+ *
+ * @throws MappingException
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Please define the FQCN here

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php
@@ -585,6 +581,8 @@ public function setDiscriminatorField($discriminatorField)
* Used for JOINED and SINGLE_TABLE inheritance mapping strategies.
*
* @param array $map
+ *
+ * @throws MappingException
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Please define the FQCN here

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php
@@ -943,6 +942,10 @@ public function setDistance($distance)
* Map a field.
*
* @param array $mapping The mapping information.
+ *
+ * @return array
+ *
+ * @throws MappingException
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Please define the FQCN here

@guilhermeblanco guilhermeblanco commented on the diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php
@@ -1444,7 +1436,7 @@ public function isInheritanceTypeCollectionPerClass()
/**
* Sets the mapped subclasses of this class.
*
- * @param array $subclasses The names of all mapped subclasses.
+ * @param string[] $subclasses The names of all mapped subclasses.
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Same concept applies here for phpDoc2 syntax

@guilhermeblanco guilhermeblanco commented on the diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php
@@ -1461,10 +1453,13 @@ public function setSubclasses(array $subclasses)
* Sets the parent class names.
* Assumes that the class names in the passed array are in the order:
* directParent -> directParentParent -> directParentParentParent ... -> root.
+ *
+ * @param string[] $classNames

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php
@@ -1515,13 +1510,16 @@ public function isIdGeneratorNone()
* value to use depending on the column type.
*
* @param array $mapping The version field mapping array
+ *
+ * @throws LockException
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Please define the FQCN here

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php
@@ -1551,12 +1549,15 @@ public function setVersionField($versionField)
* value to use depending on the column type.
*
* @param array $mapping The version field mapping array
+ *
+ * @throws LockException
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Please define the FQCN here

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Proxy/ProxyFactory.php
*/
class ProxyFactory
{
/**
- * Marker for Proxy class names.
- *
- * @var string
+ * @var DocumentManager The DocumentManager this factory is bound to.
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Please define the FQCN here

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Proxy/ProxyFactory.php
private $proxyDir;
/**
- * Used to match very simple id methods that don't need
- * to be proxied since the identifier is known.
- *
- * @var string
+ * @var ProxyGenerator the proxy generator responsible for creating the proxy classes/files.
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Please define the FQCN here

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Proxy/ProxyFactory.php
}
- if ( ! $this->dm->getMetadataFactory()->hasMetadataFor($fqn)) {
- $this->dm->getMetadataFactory()->setMetadataFor($fqn, $this->dm->getClassMetadata($className));
- }
+ $definition = $this->definitions[$className];
+ $fqcn = $definition['fqcn'];
+ $reflectionId = $definition['reflectionId'];
+ $proxy = new $fqcn($definition['initializer'], $definition['cloner']);
+ $reflectionId->setValue($proxy, $identifier);
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Missing line break before this line

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Proxy/ProxyFactory.php
continue;
}
- $proxyFileName = $this->getProxyFileName($class->name);
- $this->generateProxyClass($class, $proxyFileName, self::$proxyClassTemplate);
+ $generator = $this->getProxyGenerator();
+
+ $proxyFileName = $generator->getProxyFileName($class->getName(), $proxyDir);
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Remove line break before this line and add it after this line.
This will make a logical group of assignments and method call

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Proxy/ProxyFactory.php
continue;
}
- $proxyFileName = $this->getProxyFileName($class->name);
- $this->generateProxyClass($class, $proxyFileName, self::$proxyClassTemplate);
+ $generator = $this->getProxyGenerator();
+
+ $proxyFileName = $generator->getProxyFileName($class->getName(), $proxyDir);
+ $generator->generateProxyClass($class, $proxyFileName);
+ $generated += 1;
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Missing line break before this line

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Proxy/ProxyFactory.php
- if ( ! is_dir($parentDirectory)) {
- if (false === @mkdir($parentDirectory, 0775, true)) {
- throw ProxyException::proxyDirectoryNotWritable();
- }
- } else if ( ! is_writable($parentDirectory)) {
- throw ProxyException::proxyDirectoryNotWritable();
+ /**
+ * @return ProxyGenerator
+ */
+ public function getProxyGenerator()
+ {
+ if (null === $this->proxyGenerator) {
+ $this->proxyGenerator = new ProxyGenerator($this->proxyDir, $this->proxyNamespace);
+ $this->proxyGenerator->setPlaceholder('<baseProxyInterface>', 'Doctrine\ODM\MongoDB\Proxy\Proxy');
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Missing line break

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Proxy/ProxyFactory.php
{
- $methods = '';
+ $fqcn = ClassUtils::generateProxyClassName($className, $this->proxyNamespace);
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Please align the = signs

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Proxy/ProxyFactory.php
- $methodNames = array();
- foreach ($class->reflClass->getMethods() as $method) {
- /* @var $method ReflectionMethod */
- if ($method->isConstructor() || in_array(strtolower($method->getName()), array("__sleep", "__clone")) || isset($methodNames[$method->getName()])) {
- continue;
- }
- $methodNames[$method->getName()] = true;
+ if ( ! class_exists($fqcn, false)) {
+ $generator = $this->getProxyGenerator();
+ $fileName = $generator->getProxyFileName($className);
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Align = signs

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Proxy/ProxyFactory.php
- if ($param->isPassedByReference()) {
- $parameterString .= '&';
- }
+ $documentPersister = $this->uow->getDocumentPersister($className);
+ /* @var $reflectionId \ReflectionProperty */
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

I'd consider removing this IDE dependent comment and aligning the = signs

@guilhermeblanco guilhermeblanco and 1 other commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Proxy/ProxyFactory.php
- $parameterString .= '$' . $param->getName();
- $argumentString .= '$' . $param->getName();
+ if ($classMetadata->getReflectionClass()->hasMethod('__wakeup')) {
+ $initializer = function (Proxy $proxy) use ($documentPersister, $reflectionId) {
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Open curly braces { should be on a new line.

@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Can we turn the initializer as a private method?
It seems all we need to do is something like:

$initializer = $this->createInitializer(
    $documentPersister, 
    $reflectionId, 
    $classMetadata->getReflectionClass()->hasMethod('__wakeup')
);

And then:

function createInitializer($documentPersister, $reflectionId, $hasWakeupMethod) 
{
    return function (Proxy $proxy) use ($documentPersister, $reflectionId, $hasWakeupMethod)
    {
        $proxy->__setInitializer(null);
        $proxy->__setCloner(null);

        if ($proxy->__isInitialized()) {
            return null;
        }

        $properties = $proxy->__getLazyLoadedPublicProperties();

        foreach ($properties as $propertyName => $property) {
            if ( ! isset($proxy->$propertyName)) {
                $proxy->$propertyName = $properties[$propertyName];
            }
        }

        $proxy->__setInitialized(true);

        if ($hasWakeupMethod) {
            $proxy->__wakeup();
        }

        $id = $reflectionId->getValue($proxy);

        if (null === $documentPersister->load($id, $proxy)) {
            throw DocumentNotFoundException::documentNotFound(get_class($proxy), $id);
        }
    };
}
@Ocramius

Ocramius Jan 4, 2013

Owner

I wanted to explicitly avoid the additional conditional, that's why I got this duplication

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Proxy/ProxyFactory.php
- $methods .= ' if ($this->__isInitialized__ === false) {' . "\n";
- $methods .= ' return $this->__identifier__;' . "\n";
- $methods .= ' }' . "\n";
+ foreach ($properties as $propertyName => $property) {
+ if (!isset($proxy->$propertyName)) {
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Missing spaces around !.

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Proxy/ProxyFactory.php
- * @return bool
- */
- private function isShortIdentifierGetter($method, $class)
- {
- $identifier = lcfirst(substr($method->getName(), 3));
- $cheapCheck = (
- $method->getNumberOfParameters() == 0 &&
- substr($method->getName(), 0, 3) == "get" &&
- $class->identifier === $identifier &&
- $class->hasField($identifier) &&
- (($method->getEndLine() - $method->getStartLine()) <= 4)
- && in_array($class->fieldMappings[$identifier]['type'], array('id', 'custom_id'))
- );
+ $proxy->__setInitialized(true);
+ $proxy->__wakeup();
+ $id = $reflectionId->getValue($proxy);
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Missing line break before this line

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Proxy/ProxyFactory.php
- if ($cheapCheck) {
- $code = file($method->getDeclaringClass()->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 (null === $documentPersister->load($id, $proxy)) {
+ throw DocumentNotFoundException::documentNotFound(get_class($proxy), $id);
+ }
+ };
+ } else {
+ $initializer = function (Proxy $proxy) use ($documentPersister, $reflectionId) {
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Curly braces on a new line

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Proxy/ProxyFactory.php
- if ($class->reflClass->hasMethod('__sleep')) {
- $sleepImpl .= "return array_merge(array('__isInitialized__'), parent::__sleep());";
- } else {
- $sleepImpl .= "return array('__isInitialized__', ";
- $first = true;
-
- foreach ($class->getReflectionProperties() as $name => $prop) {
- if ($first) {
- $first = false;
- } else {
- $sleepImpl .= ', ';
+ foreach ($properties as $propertyName => $property) {
+ if (!isset($proxy->$propertyName)) {
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Missing spaces around !.

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Proxy/ProxyFactory.php
}
- $sleepImpl .= "'" . $name . "'";
- }
+ $proxy->__setInitialized(true);
+ $id = $reflectionId->getValue($proxy);
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Missing line break

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Proxy/ProxyFactory.php
- /** @private */
- public function __load()
- {
- if (!$this->__isInitialized__ && $this->__documentPersister__) {
- $this->__isInitialized__ = true;
-
- if (method_exists($this, "__wakeup")) {
- // call this after __isInitialized__to avoid infinite recursion
- // but before loading to emulate what ClassMetadata::newInstance()
- // provides.
- $this->__wakeup();
- }
-
- if ($this->__documentPersister__->load($this->__identifier__, $this) === null) {
- throw \Doctrine\ODM\MongoDB\DocumentNotFoundException::documentNotFound(get_class($this), $this->__identifier__);
+ $cloner = function (Proxy $proxy) use ($documentPersister, $classMetadata, $reflectionId) {
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Curly braces on a new line

@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Can we also take the same approach I proposed for initializer (isolate in a private method)?
This turns this method into a creator of definitions, compatible with SOLID principles by having a single responsibility. =)

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Proxy/ProxyFactory.php
- /** @private */
- public function __isInitialized()
- {
- return $this->__isInitialized__;
- }
+ $proxy->__setInitialized(true);
+ $proxy->__setInitializer(null);
+ $id = $reflectionId->getValue($proxy);
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Missing line breaks
Align = signs

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/Proxy/ProxyFactory.php
- public static function generateProxyClassName($className, $proxyNamespace)
- {
- return rtrim($proxyNamespace, '\\') . '\\'.self::MARKER.'\\' . ltrim($className, '\\');
+ $this->definitions[$className] = array(
+ 'fqcn' => $fqcn,
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Do we need all these spaces?

@guilhermeblanco guilhermeblanco commented on an outdated diff Nov 22, 2012

lib/Doctrine/ODM/MongoDB/UnitOfWork.php
@@ -1990,6 +1990,7 @@ private function doMerge($document, array &$visited, $prevManagedCopy = null, $a
$prop->setValue($managedCopy, $other);
} else {
$targetDocument = isset($assoc2['targetDocument']) ? $assoc2['targetDocument'] : get_class($other);
+ /* @var $targetClass \Doctrine\ODM\MongoDB\Mapping\ClassMetadata */
@guilhermeblanco

guilhermeblanco Nov 22, 2012

Owner

Remove this.
Align = signs

Owner

guilhermeblanco commented Nov 22, 2012

Also, don't forget to make the tests pass

@Ocramius Ocramius commented on the diff Jan 12, 2013

composer.json
@@ -14,7 +14,7 @@
"require": {
"php": ">=5.3.2",
"symfony/console": ">=2.0-dev,<2.3-dev",
- "doctrine/common": ">=2.2.0,<2.5-dev",
+ "doctrine/common": ">=2.4-dev,<2.5-dev",
@Ocramius

Ocramius Jan 12, 2013

Owner

This requires a "minimum-stability": "dev" bump until 2.4 is tagged

Ocramius closed this Feb 19, 2013

Ocramius deleted the Ocramius:DCOM-96 branch Jan 24, 2014

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