On Tue, Jun 4, 2024 at 10:23 PM Tim Düsterhus <t...@bastelstu.be> wrote: > > Hi > > On 6/4/24 14:28, Nicolas Grekas wrote: > > Please find all the details here: > > https://wiki.php.net/rfc/lazy-objects > > > > We look forward to your thoughts and feedback. > > I've gave the RFC three or four passes and I'm not quite sure if I > follow everything, here's a list of some questions / remarks that came > to mind, roughly ordered by the order of things appearing in the RFC. > > - "been tested successfully on the Doctrine and on the Symfony projects" > > Is there a PoC patch showcasing how the code would change / be > simplified for those pre-existing codebases? > > - int $options = 0 > > Not a fan of flag parameters that take a bitset, those provide for a > terrible DX due to magic numbers. Perhaps make this a regular (named) > parameter, or an list of enum LazyObjectOptions { case > SkipInitOnSerialize; }? > > - skipProperty() > > Not a fan of the method name, because it doesn't really say what it > does, without consulting the docs. Perhaps `skipInitializationFor()` or > similar? > > - setProperty() > > Not a fan of the method name, because it is not a direct counterpart to > `getProperty()`. Unfortunately I don't have a better suggestion. > > - The examples should be expanded and clarified, especially the one for > makeLazyProxy(): > > My understanding is that the $object that is passed to the first > parameter of makeLazyProxy() is completely replaced. Is this > understanding correct? What does that mean for spl_object_hash(), > spl_object_id()? What does this mean for WeakMap and WeakReference? What > does this mean for objects that are only referenced from within $object? > > Consider this example: > > class Foo { > public function __destruct() { echo __METHOD__; } > } > > class Bar { > public string $s; > public ?Foo $foo; > > public function __destruct() { echo __METHOD__; } > } > > $bar = new Bar(); > $bar->foo = new Foo(); > > ReflectionLazyObject::makeLazyProxy($bar, function (Bar $bar) { > $result = new Bar(); > $result->foo = null; > $result->s = 'init'; > return $result; > }); > > var_dump($bar->s); > > My understanding is that this will dump `string(4) "init"`. Will the > destructor of Foo be called? Will the destructor of Bar be called? > > - What happens if I make an object lazy that already has all properties > initialized? Will that be a noop? Will that throw? Will that create a > lazy object that will never automatically be initialized? > > - Cloning, unless __clone() is implemented and accesses a property. > > The semantics of cloning a lazy object should be explicitly spelled out > in the RFC, ideally with an example of the various edge cases (should > any exist). > > - Before calling the initializer, properties that were not initialized > with ReflectionLazyObject::skipProperty(), > ReflectionLazyObject::setProperty(), > ReflectionLazyObject::setRawProperty() are initialized to their default > value. > > Should skipProperty() also skip the initialization to the default value? > My understanding is that it allows skipping the initialization on > access, but when initialization actually happens it should probably be > set to a well-defined value, no? > > Am I also correct in my understanding that this should read "initialized > to their default value (if any)", meaning that properties without a > default value are left uninitialized? > > - If an exception is thrown while calling the initializer, the object is > reverted to its pre-initialization state and is still considered lazy. > > Does this mean that the initializer will be called once again when > accessing another property? Will the "revert to its pre-initialization" > work properly when you have nested lazy objects? An example would > probably help. > > - The initializer is called with the object as first parameter. > > What is the behavior of accessing the object properties, while the > initializer is active? Based on the examples, I assume it will not be > recursively called, similarly to how the hooks work? > > - The object is marked as non-lazy and the initializer is released. > > What does it mean for the initializer to be released? Consider the > following example: > > ReflectionLazyObject::makeLazyGhost($o, $init = function ($o) use > (&$init) { > $o->init = $init; > }); > > - The return value of the initializer has to be an instance of a parent > or a child class of the lazy-object and it must have the same properties. > > Would returning a parent class not violate the LSP? Consider the > following example: > > class A { public string $s; } > class B extends A { public function foo() { } } > > $o = new B(); > ReflectionLazyObject::makeLazyProxy($o, function (B $o) { > return new A(); > }); > > $o->foo(); // works > $o->s = 'init'; > $o->foo(); // breaks > > - The destructor of lazy non-initialized objects is not called. > > That sounds unsafe. Consider the following example: > > class Mutex { > public string $s; > public function __construct() { > // take lock > } > > public function __destruct() { > // release lock > } > } > > $m = new Mutex(); > ReflectionLazyObject::makeLazyGhost($m, function ($m) { > }); > unset($m); // will not release the lock. > > - Using the Manager::createManager() factory is not compatible with > ghost objects because their initializer requires initializing the ghost > object in place, > > I don't understand that example, because it doesn't actually use lazy > objects and thus I don't understand if Manager, Dispatcher, or both are > intended to be lazily initialized. It would help to rewrite the example > to use `makeLazyGhost()` and indicate with a comment in which cases the > problem would arise. > > - Backward Incompatible Changes > > There a technicality: The `ReflectionLazyObject` class name will no > longer be available to userland. > > Best regards > Tim Düsterhus
As someone who has had to maintain these proxies/ghosts before, this looks quite useful and powerful. I feel it has rather wonky syntax, but it is clearly better than the alternative of implementing it yourself. I'm also a huge fan that the syntax allows for usage/creation far and away from the definition/class itself. Good luck, and I hope it passes. Robert Landers Software Engineer Utrecht NL
