On Fri, Jun 14, 2024, at 12:13 PM, Arnaud Le Blanc wrote:
> Hi Tim,
>
> We have updated the RFC to address your feedback. Please find
> additional answers below.
The updated RFC looks much better, thank you. Though I still have some
thoughts, in no particular order.
> The actual instance is allowed to escape the proxy and to create direct
> references to itself.
How? Is this a "return $this" type of situation? This could use more fleshing
out and examples.
The terms "virtual" and "proxy" seem to be used interchangeably in different
places, including in the API. Please just use one, and purge the other. It's
confusing as is. :-) (I'd favor "proxy", as it seems more accurate to what is
happening.) For that matter, I'd be very tempted to remove the word "lazy"
from the API calls. `newGhostInstance()` and `newProxyInstance()` are plenty
understandable, and shorter/easier to read. (Similarly, `makeGhostInstance()`
and `makeProxyInstance()`. Although since those are more about modifying a
provided object to be lazy, perhaps "make" isn't the right verb to use as that
often means "create".)
Under Common Behavior, you have an example of calling the constructor directly,
using the reflection API, but not of binding the callable, which the text says
is also available. Please include an example of that so we can evaluate how
clumsy (or not) it would be.
> After calling newLazyGhostInstance(), the behavior of the object is the same
> as an object created by newLazyGhostInstance().
I think the first is supposed be a make* call?
> When making an existing object lazy, the makeInstanceLazy*() methods call the
> destructor unless the SKIP_DESTRUCTOR flag is given.
I don't quite get why this is. Admittedly destructors are rarely used, but why
does it need to call the destructor?
I find it interesting that your examples list DICs as a use case for proxies,
when I would have expected that to fit ghosts better. The common pattern, I
would think, would be:
class Service {
public function __construct(private ServiceA $a, private ServiceB $b) {}
}
$c = some_container();
$init = fn() => $this->__construct($c->get(ServiceA::class),
$c->get(ServiceB::class));
$service = new ReflectionLazyObjectFactory(Service::class, $init);
(Most likely in generated code that can dynamically sort out the container
calls to inline.)
Am I missing something?
ReflectionLazyObjectFactory is a terrible name. Sorry, it is. :-) Especially
if it's subclassing ReflectionClass. If it were its own thing, maybe, but it's
still too verbose. I know you don't want to put more on the "dumping ground"
fo ReflectionClass, but honestly, that feels more ergonomic to me. That way
the following are all siblings:
newInstance(...$args)
newInstanceWithoutConstructor(...$args)
newGhostInstance($init)
newProxyInstance($init)
That feels a lot more sensible and ergonomic to me. isInitialized(),
initialized(), etc. also feel like they make more sense as methods on
ReflectionObject, not as static methods on a random new class.
As I said, definitely improved, and I like where it's going. I think it can
improve further, though.
--Larry Garfield
