Adding failing test for Ocramius/ProxyManager#116 - public properties defaults are ignored

[Ocramius]

This is a hotfix for #116

[blanchonvincent]

@Ocramius I don't understand this line, do you force initializer to be null ? It's a BC and a problem because sometimes we need to keep initializer not null no ?

[Ocramius]

@blanchonvincent setting multiple properties is going to trigger initialization multiple times via __set(). That's why I have to set the initializer to null

Got any alternative approach?

[blanchonvincent]

@Ocramius maybe use isset() to check if property if initialized ? or add a static flag in __call method ?

[Ocramius]

@blanchonvincent the initializer is always either Closure or null - can't be false. The other way would be to have another private property that is a flag like "I'm currently initializing myself, don't recurse initialization", but it's kinda bloated.

I'll think about it before merging though, good catch on the BC break!

[blanchonvincent]

@Ocramius I thought about the isset with the line below :

if (!isset(\$this->\$key)) {
    \$this->\$key = \$default;
}

instead of

\$this->\$key = \$default;

[Ocramius]

@blanchonvincent at this point, isset($this->$key) will trigger calls to __isset() for each property

[blanchonvincent]

@Ocramius ha ha right ! sorry, it was a bad idea. The flag can be cool but it's annoying

[Ocramius]

Something like:

if ($this->startedInitializing230874208y80) {
    return;
}

$this->startedInitializing230874208y80 = true;

// do other things

$this->initializer0837082470397409->__invoke(...);
$this->startedInitializing230874208y80 = false;

Think it's acceptable?

[Ocramius]

@blanchonvincent I included a test to fix the bc break

[blanchonvincent]

@Ocramius Why ? If initialization is set to false, public properties will set each time there is a method call no ?

[Ocramius]

Ouch! Good point! Will fix :-)

[blanchonvincent]

@Ocramius Is there no change about this line before the merge ?

[Ocramius]

@blanchonvincent didn't find any other breakages, and added tests to verify that your use cases are covered ;)

[blanchonvincent]

@Ocramius try this plz : https://gist.github.com/blanchonvincent/7699952 (there is also an other error with a problem with a reference)

[Ocramius]

I have an example for multiple initialization, but it is indeed broken. An initializer like:

function (BaseClass $proxy, $method, $parameters, & $initializer) use ($identifier, $someRepo) {
    if ($proxy->isInitialized() && ! $someRepo->checkChanged($identifier)) {
        return;
    }

     // reset properties
};

That's the kind of "advanced" usage that I can think of, but it would not really work with public properties...

[Ocramius]

As for setting the initializer always to null, I'm +1 for that, but only in another major. Mind opening an issue with that?

[Ocramius]

A fix for the broken behavior that I've shown in #122 (comment) may be to unset all public properties AGAIN when triggering initialization.

[Ocramius]

@blanchonvincent I opened #126 with the failure you randomly discovered with the non-existing property :-)

[blanchonvincent]

👍 thanks

[coveralls]

Coverage increased (+0.04%) when pulling b85dc27 on hotfix/issue-#116-public-properties-defaults into 44d82e7 on master.