Lazy initialization should (probably) be denied if a value holder already contains a value

[Ocramius]

Currently, lazy initialization happens through following pseudo code:

$this->initializer && $this->initializer->__invoke();

It could be enhanced by using following code:

null !== $this->wrapped && $this->initializer && $this->initializer->__invoke();

This disallows invalid usage of the initializer

[magnusnordlander]

Would you consider an initializer like this "invalid usage"?

// The factory is a LazyLoadingValueHolderFactory
$this->factory->createProxy(
    $modelClass,
    function (&$wrappedInstance, LazyLoadingInterface $proxy, $method) use ($id, $modelClass) {
        if ($method == 'getId') {
            // IdContainer is a class containing just a getId method which returns the ID from the constructor
            $wrappedInstance = new IdContainer($id);
        } else {
            $proxy->setProxyInitializer(null);

            // The find method is of course quite expensive
            $wrappedInstance = $this->getRepository($modelClass)->find($id);
        }

        return true;
    }
);

Because an initializer like this wouldn't work if the proposed change is implemented, and it's quite useful (I mean, why do an expensive call to get a piece of information we already have).

Of course, a better solution might be to create some facility for non-faulting methods on proxies, but having looked in to that it seems to be somewhat complex to accomplish...

[Ocramius]

@magnusnordlander I see what you did there, and it makes kind-of sense, but wouldn't this be better suited for a ghost object?

Not to mention that a new IdContainer($id) will be instantiated on any method call anyway...

I'm actually thinking of including facilities (for the ghost object, not for virtual proxies/value holders) to exclude properties/methods from triggering lazy loading. That would allow building quite nice partial objects: is that what you are looking for?

[magnusnordlander]

@Ocramius Not in this particular use case. The object I'm proxying is weirdly designed and not well suited for a ghost object.

Why would an IdContainer be instantiated on any method call other than getId? It should just branch to the else-branch? The find method doesn't return an IdContainer, but rather an instance of the weird object I'm proxying (which knows nothing about IdContainer).

Excluding methods from triggering lazy loading on ghost objects would however work very well for the proxies in another project I'm maintaining, e-butik/gittern, so I'd still consider it an excellent idea :)

[Ocramius]

@magnusnordlander ah, I see... You can eventually prevent the continuous instantiation by tweaking the initializer as following:

// The factory is a LazyLoadingValueHolderFactory
$this->factory->createProxy(
    $modelClass,
    function (& $wrappedInstance, LazyLoadingInterface $proxy, $method) use ($id, $modelClass) {
        if ($method == 'getId') {
            // IdContainer is a class containing just a getId method which returns the ID from the constructor
            $wrappedInstance || ($wrappedInstance = new IdContainer($id));

            return true;
        }

        $proxy->setProxyInitializer(null);

        // The find method is of course quite expensive
        $wrappedInstance = $this->getRepository($modelClass)->find($id);

        return true;
    }
);

That would probably work.

So yes, you probably hereby have the use case that I was missing to deny this optimization. Are you eventually able to write a small functional test to protect this behaviour? I don't want to break it :)

[Ocramius]

@magnusnordlander for the ghost object optimization thing, I actually want to introduce it for the next minor release since it's the closing gap between doctrine/common and ocramius/proxy-manager. Then doctrine project MAY consider getting this project under its name :)

[magnusnordlander]

@Ocramius Will do

[Ocramius]

I decided that for 1.x we'll make the initializer null by default after it is triggered. I'll think about a way of allowing multiple initialization.

There's two main ways of dealing with this imo:

With a byref param:

$newInitializer = null;
$this->initializer->__invoke(..., $newInitializer, ...);
$this->initializer = $newInitializer;

Using the return value of the initializer directly:

$this->initializer = $this->initializer->__invoke(..., $newInitializer, ...);

Not yet sure on the best approach

[Ocramius]

Ping @blanchonvincent

[blanchonvincent]

@Ocramius currently, in the documentation, initializer do return true;. So will there be conflicts if you choose the usage of the return value of the initializer directly ?

[Ocramius]

@blanchonvincent that flag is just a convention right now... It's not really relevant :-)

[blanchonvincent]

@Ocramius I already asked the question (sorry for that) but I don't understand why there is a return which never catched ?

You said to me

there's use cases for that (copy-on-write and copy-on-read) but they're edge cases. Still, it's a fairly simple approach for any use case for immutable objects, mainly

But I think I don't understand yet

[Ocramius]

The return value is just a "success" flag. I might remove it, since I ended up not using it at all ;)

[blanchonvincent]

ok :)