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

Already on GitHub? Sign in to your account

Hotfix/doctrine/common#247 fixes #248

Merged
merged 2 commits into from Jan 26, 2013

Conversation

Projects
None yet
5 participants
Owner

Ocramius commented Jan 26, 2013

Includes fixes suggested for #247 (plus a weird fix on a failure with array_map on PHP <5.4)

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/DCOM-167

Oh btw, I just (automatically) realized that you are not creating this pull request against the master branch.

Unless there are good reasons for this, i would suggest to close and rebase the Pull Request against master and then create it again. Sorry!

@stof stof commented on an outdated diff Jan 26, 2013

...trine/Tests/Common/Proxy/AbstractProxyFactoryTest.php
$proxyGenerator->expects($this->once())->method('getProxyFileName');
$proxyGenerator->expects($this->once())->method('generateProxyClass');
- $manager = $this->getMock('Doctrine\Common\Persistence\ObjectManager');
-
- $proxyFactory = $this->getMock(
+ $metadataFactory = $this->getMock('Doctrine\Common\Persistence\Mapping\ClassMetadataFactory');
+ $proxyFactory = $this->getMock(
@stof

stof Jan 26, 2013

Member

you could use getMockForAbstractClass which automatically set the mocked methods (all abstract ones)

@stof stof commented on an outdated diff Jan 26, 2013

...trine/Tests/Common/Proxy/AbstractProxyFactoryTest.php
@@ -78,5 +80,23 @@ public function testResetUnitializedProxy()
$proxyFactory->resetUninitializedProxy($proxy);
}
+
+ public function testDisallowsResettingInitializedProxy()
+ {
+ $proxy = $this->getMock('Doctrine\Common\Proxy\Proxy');
@stof

stof Jan 26, 2013

Member

too much spaces

@guilhermeblanco guilhermeblanco commented on the diff Jan 26, 2013

lib/Doctrine/Common/Proxy/AbstractProxyFactory.php
{
- $definition = $this->getProxyDefinition($className);
+ $definition = isset($this->definitions[$className])
@guilhermeblanco

guilhermeblanco Jan 26, 2013

Owner
$definition = isset($this->definitions[$className])
    ? $this->definitions[$className] 
    : $this->getProxyDefinition($className);

$fqcn  = $definition->proxyClassName;
$proxy = new $fqcn($definition->initializer, $definition->cloner);
@Ocramius

Ocramius Jan 26, 2013

Owner

@guilhermeblanco when you comment those, do it on the line that is gonna be changed ;)

@guilhermeblanco

guilhermeblanco Jan 26, 2013

Owner

I did, but you also updated the diff at the same time. =)

@guilhermeblanco guilhermeblanco commented on the diff Jan 26, 2013

lib/Doctrine/Common/Proxy/AbstractProxyFactory.php
}
$className = ClassUtils::getClass($proxy);
- $definition = $this->getProxyDefinition($className);
+ $definition = isset($this->definitions[$className])
@guilhermeblanco

guilhermeblanco Jan 26, 2013

Owner
$definition = isset($this->definitions[$className])
    ? $this->definitions[$className] 
    : $this->getProxyDefinition($className);
@beberlei

beberlei Jan 26, 2013

Owner

why is this here anyways? its done in getProxyDefinition again.

@Ocramius

Ocramius Jan 26, 2013

Owner

In fact, I'm moving the check out of getProxyDefinition

@guilhermeblanco guilhermeblanco commented on an outdated diff Jan 26, 2013

lib/Doctrine/Common/Proxy/AbstractProxyFactory.php
- if ( ! class_exists($proxyClassName, false)) {
- $fileName = $this->proxyGenerator->getProxyFileName($className);
+ $classMetadata = $this->metadataFactory->getMetadataFor($className);
+ $className = $classMetadata->getName(); // aliases and case sensitivity
+ $this->definitions[$className] = $this->createProxyDefinition($className);
@guilhermeblanco

guilhermeblanco Jan 26, 2013

Owner

Missing line break before and after this statement.
Also, when you do this, you need to realign the = sign on other blocks

@guilhermeblanco guilhermeblanco and 1 other commented on an outdated diff Jan 26, 2013

...trine/Tests/Common/Proxy/AbstractProxyFactoryTest.php
- $proxy = $this->getMock('Doctrine\Common\Proxy\Proxy');
- $definition = new ProxyDefinition(get_class($proxy), array(), array(), null, null);
+ $metadataFactory->expects($this->once())->method('getMetadataFor')->will($this->returnValue($metadata));
@guilhermeblanco

guilhermeblanco Jan 26, 2013

Owner

Chaining is the same as a scope increment. It needs to earn one indentation level.
Also, chaining calls on a single line is prohibited in Object Calisthenics.
Here is the code updated:

$metadataFactory
    ->expects($this->once())
    ->method('getMetadataFor')
    ->will($this->returnValue($metadata));
@Ocramius

Ocramius Jan 26, 2013

Owner

There's loads of these all over the test suite(s) and I personally think it becomes totally unreadable, especially when the pattern is always

$blih->expects($bloh)->method($blah)->with($bluh)->will($bleh)

@guilhermeblanco

guilhermeblanco Jan 26, 2013

Owner

But still our codebase should be consistent. =)

@Ocramius

Ocramius Jan 26, 2013

Owner

Meh =_= Ok, fixing. You missed a couple of these some lines above :P

@guilhermeblanco guilhermeblanco commented on an outdated diff Jan 26, 2013

...trine/Tests/Common/Proxy/AbstractProxyFactoryTest.php
- $proxyGenerator = $this->getMock('Doctrine\Common\Proxy\ProxyGenerator', array(), array(), '', false);
- $manager = $this->getMock('Doctrine\Common\Persistence\ObjectManager');
- $manager->expects($this->once())->method('getClassMetadata')->will($this->returnValue($metadata));
+ $metadataFactory->expects($this->once())->method('getMetadataFor')->will($this->returnValue($metadata));

@guilhermeblanco guilhermeblanco commented on an outdated diff Jan 26, 2013

...trine/Tests/Common/Proxy/AbstractProxyFactoryTest.php
@@ -78,5 +77,17 @@ public function testResetUnitializedProxy()
$proxyFactory->resetUninitializedProxy($proxy);
}
+
+ public function testDisallowsResettingInitializedProxy()
+ {
+ $proxyFactory = $this->getMockForAbstractClass('Doctrine\Common\Proxy\AbstractProxyFactory', array(), '', false);
+ $proxy = $this->getMock('Doctrine\Common\Proxy\Proxy');
+
+ $proxy->expects($this->any())->method('__isInitialized')->will($this->returnValue(true));

@beberlei beberlei commented on an outdated diff Jan 26, 2013

...trine/Tests/Common/Proxy/AbstractProxyFactoryTest.php
@@ -5,46 +5,46 @@
use Doctrine\Tests\DoctrineTestCase;
use Doctrine\Common\Proxy\ProxyDefinition;
+/**
+ * @covers \Doctrine\Common\Proxy\AbstractPRoxyFactory
+ * @covers \Doctrine\Common\Proxy\ProxyDefinition
@beberlei

beberlei Jan 26, 2013

Owner

please remove them, we never worked with covers before i dont want to start this micromanagement

beberlei added a commit that referenced this pull request Jan 26, 2013

@beberlei beberlei merged commit 9c9c3c1 into doctrine:ProxyFactory Jan 26, 2013

1 check was pending

default The Travis build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment