Skip to content

Loading…

Proxies shouldn't serialize static properties in __sleep() #296

Merged
merged 2 commits into from

3 participants

@mnapoli

This PR contains a test and a fix for the following bug: Proxies did serialize static properties.

I believe this is a regression in 2.4 since I've never met this bug before.

Given the class:

class StaticPropertyClass
{
    protected static $protectedStaticProperty;
}

Before the fix, proxies would contain the following __sleep method:

    public function __sleep()
    {
        if ($this->__isInitialized__) {
            return array('__isInitialized__', 'protectedStaticProperty');
        }

        return array('__isInitialized__', 'protectedStaticProperty');
    }

With the fix:

    public function __sleep()
    {
        if ($this->__isInitialized__) {
            return array('__isInitialized__');
        }

        return array('__isInitialized__');
    }
@doctrinebot

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DCOM-212

We use Jira to track the state of pull requests and the versions they got
included in.

@mnapoli mnapoli commented on the diff
lib/Doctrine/Common/Proxy/ProxyGenerator.php
@@ -619,6 +619,10 @@ public function __sleep()
/* @var $prop \ReflectionProperty */
foreach ($class->getReflectionClass()->getProperties() as $prop) {
+ if ($prop->isStatic()) {
+ continue;
+ }
@mnapoli
mnapoli added a note

I would have preferred to use filters on getProperties(), but not possible

@Ocramius Doctrine member

Looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Ocramius Ocramius commented on the diff
...ctrine/Tests/Common/Proxy/ProxyClassGeneratorTest.php
@@ -118,6 +118,24 @@ public function testClassWithSleepProxyGeneration()
$this->assertEquals(1, substr_count($classCode, 'parent::__sleep()'));
}
+ /**
+ * Check that the proxy doesn't serialize static properties (in __sleep() method)
@Ocramius Doctrine member

Can you add @group DCOM-212 to the docblock?

@mnapoli
mnapoli added a note

done (just below)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Ocramius Ocramius merged commit 4233262 into doctrine:master

1 check passed

Details default The Travis CI build passed
@mnapoli

Would it be possible to do a minor release with that fix? There has not been any release since september 2013.

Thanks!

@Ocramius Ocramius self-assigned this
@mnapoli mnapoli deleted the myclabs:ProxyStaticProperties branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
4 lib/Doctrine/Common/Proxy/ProxyGenerator.php
@@ -619,6 +619,10 @@ public function __sleep()
/* @var $prop \ReflectionProperty */
foreach ($class->getReflectionClass()->getProperties() as $prop) {
+ if ($prop->isStatic()) {
+ continue;
+ }
@mnapoli
mnapoli added a note

I would have preferred to use filters on getProperties(), but not possible

@Ocramius Doctrine member

Looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
$allProperties[] = $prop->isPrivate()
? "\0" . $prop->getDeclaringClass()->getName() . "\0" . $prop->getName()
: $prop->getName();
View
19 tests/Doctrine/Tests/Common/Proxy/ProxyClassGeneratorTest.php
@@ -118,6 +118,25 @@ public function testClassWithSleepProxyGeneration()
$this->assertEquals(1, substr_count($classCode, 'parent::__sleep()'));
}
+ /**
+ * Check that the proxy doesn't serialize static properties (in __sleep() method)
@Ocramius Doctrine member

Can you add @group DCOM-212 to the docblock?

@mnapoli
mnapoli added a note

done (just below)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ * @group DCOM-212
+ */
+ public function testClassWithStaticPropertyProxyGeneration()
+ {
+ if (!class_exists('Doctrine\Tests\Common\ProxyProxy\__CG__\StaticPropertyClass', false)) {
+ $className = 'Doctrine\Tests\Common\Proxy\StaticPropertyClass';
+ $metadata = $this->createClassMetadata($className, array());
+ $proxyGenerator = new ProxyGenerator(__DIR__ . '/generated', __NAMESPACE__ . 'Proxy', true);
+
+ $this->generateAndRequire($proxyGenerator, $metadata);
+ }
+
+ $classCode = file_get_contents(__DIR__ . '/generated/__CG__DoctrineTestsCommonProxyStaticPropertyClass.php');
+ $this->assertEquals(1, substr_count($classCode, 'function __sleep'));
+ $this->assertNotContains('protectedStaticProperty', $classCode);
+ }
+
private function generateAndRequire($proxyGenerator, $metadata)
{
$proxyGenerator->generateProxyClass($metadata, $proxyGenerator->getProxyFileName($metadata->getName()));
View
11 tests/Doctrine/Tests/Common/Proxy/StaticPropertyClass.php
@@ -0,0 +1,11 @@
+<?php
+
+namespace Doctrine\Tests\Common\Proxy;
+
+/**
+ * Test asset class
+ */
+class StaticPropertyClass
+{
+ protected static $protectedStaticProperty;
+}
Something went wrong with that request. Please try again.