On Thu, Jun 20, 2024 at 10:52 AM Nicolas Grekas <
nicolas.grekas+...@gmail.com> wrote:
>
>
> Le mar. 18 juin 2024 à 22:59, Larry Garfield <la...@garfieldtech.com> a
> écrit :
>
>> On Tue, Jun 18, 2024, at 5:45 PM, Arnaud Le Blanc wrote:
>> > Hi Larry,
>> >
>> > Following your feedback we propose to amend the API as follows:
>> >
>> > ```
>> > class ReflectionClass
>> > {
>> > public function newLazyProxy(callable $factory, int $options):
>> object {}
>> >
>> > public function newLazyGhost(callable $initializer, int $options):
>> object {}
>> >
>> > public function resetAsLazyProxy(object $object, callable
>> > $factory, int $options): void {}
>> >
>> > public function resetAsLazyGhost(object $object, callable
>> > $initializer, int $options): void {}
>> >
>> > public function initialize(object $object): object {}
>> >
>> > public function isInitialized(object $object): bool {}
>> >
>> > // existing methods
>> > }
>> >
>> > class ReflectionProperty
>> > {
>> > public function setRawValueWithoutInitialization(object $object,
>> > mixed $value): void {}
>> >
>> > public function skipInitialization(object $object): void {}
>> >
>> > // existing methods
>> > }
>> > ```
>> >
>> > Comments / rationale:
>> > - Adding methods on ReflectionClass instead of ReflectionObject is
>> > better from a performance point of view, as mentioned earlier
>> > - Keeping the word "Lazy" in method names is clearer, especially for
>> > "newLazyProxy" as a the "Proxy" pattern has many uses-cases that are
>> > not related to laziness. However we removed the word "Instance" to
>> > make the names shorter.
>> > - We have renamed "make" methods to "reset", following your feedback
>> > about the word "make". It should better convey the behavior of these
>> > methods, and clarify that it's modifying the object in-place as well
>> > as resetting its state
>> > - setRawValueWithoutInitialization() has the same behavior as
>> > setRawValue() (from the hooks RFC), except it doesn't trigger
>> > initialization
>> > - Renamed $initializer to $factory for proxy methods
>> >
>> > WDYT?
>> >
>> > Best Regards,
>> > Arnaud
>>
>> Oh, that looks *so* much more self-explanatory and readable. I love it.
>> Thanks! (Looks like the RFC text hasn't been updated yet.)
>>
>
> Happy you like it so much! The text of the RFC is now up to date. Note
> that we renamed ReflectionProperty::skipInitialization() and
> setRawValueWithoutInitialization() to skipLazyInitialization() and
> setRawValueWithoutLazyInitialization() after we realized that
> ReflectionProperty already has an isInitialized() method for something
> quite different.
>
> While Arnaud works on moving the code to the updated API, are there more
> comments on this RFC before we consider opening the vote?
>
Thank you for updating the API, the RFC is now much easier to grasp.
My few comments on the updated RFC:
1 ) ReflectionClass API is already very large, adding methods should use
naming carefully to make sure that users identify them as belonging to a
sub.feature (lazy objects) in particular, so i would prefer we rename some
of the new methods to:
isInitialized => isLazyObject (with inverted logic)
initialize => one of initializeLazyObject / initializeWhenLazy /
lazyInitialize - other methods in this RFC are already very outspoken, so I
don’t mind being very specific here as well.
The reason is „initialized“ is such a generic word, best not have API users
make assumptions about what this relates to (readonly, lazy, …)
2.) I am 100% behind the implementation of lazy ghosts, its really great
work with all the behaviors. Speaking with my Doctrine ORM core developer
hat this has my full support.
3.) the lazy proxies have me worried that we are opening up a can of worms
by having the two objects and the magic of using only the properties of one
and the methods of the other.
Knowing Symfony DIC, the use case of a factory method for the proxy is a
compelling argument for having it, but it is a leaky abstraction solving
the identity issue only on one side, but the factory code might not know
its used for a proxy and make all sorts of decisions based on identity that
lead to problems.
Correct me if i am wrong or missing something, but If the factory does not
know about proxying, then it would also be fine to build a lazy ghost and
copy over all state after using the factory. This creates a similar amount
of problems with identity, but is less magic while doing so. All of the
inheritance heirachy and properties must exist logic can also be
implemented in the userland initializer, passing the responsibility for the
mess over to userland ;)
class Container
{
public function getClientService(): Client
{
$reflector = new ReflectionClass(Client::class);
$client = $reflector->newLazyGhost(function (Client $ghost) use
($container) {
$clientFactory = $container->get('client_factory');
$client = $clientFactory->createClient();
// not sure this is 100% right, the idea is to copy all state over
$vars = get_mangled_object_vars($client);
foreach ($vars as $k => $v) { $ghost->$k = $v; }
});
return $client;
}
This would also allow to make „initialize“ return void and simplify this
part of the API.
4.) I am wondering, do we need the resetAs* methods? You can already
implement lazy proxies in userland code by manually writing the code, we
don’t need engine support for that. Not having these two methods would
reduce the surface of the RFC / API considerably. And given the „real
world“ example is not really real world, only the Doctrine
(createLazyGhost) and Symfony (createLazyGhost or createLazyProxy) are,
this shows maybe its not needed.
5.) The RFC does not spell it out, but I assume this does not have any
effect on stacktraces, i.e. since properties are proxied, there are no
„magic“ frames appearing in the stacktraces?
>
> Cheers,
> Nicolas
>
