Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Typed property /w magic __get cannot be uninitialized #9389

Open
mvorisek opened this issue Aug 20, 2022 · 6 comments
Open

Typed property /w magic __get cannot be uninitialized #9389

mvorisek opened this issue Aug 20, 2022 · 6 comments

Comments

@mvorisek
Copy link
Contributor

mvorisek commented Aug 20, 2022

Description

demo: https://3v4l.org/lDeXi

Currently, typed property can be "almost uninitialized" by unset().

But doing such uninitialization unsets the property and allows magic __get to kick in. This is expected and discussed in #9021.

Because of this, non-readonly typed property cannot be uninitialized properly. This is an issue in cases when the same object is to be restored, reused or cleaned.

To allow an object state to be restored/uninitialized correctly, I propose to add ReflectionProperty::setUninitialized() method and backport it into PHP 8.0 and above.

@iluuu1994
Copy link
Member

That message indeed look fishy. I'm not even sure if the type check is intentional. I'll check the RFC and implementation later.

@iluuu1994 iluuu1994 assigned iluuu1994 and unassigned iluuu1994 Aug 20, 2022
@iluuu1994
Copy link
Member

Meant to comment this on the other issue.

@mvorisek
Copy link
Contributor Author

Meant to comment this on the other issue.

Yes, the wrong message issue is reported in #9388.

I'll check the RFC and implementation later.

The __get return type validation againt typed property is mentioned in https://wiki.php.net/rfc/typed_properties_v2#overloaded_properties. This is also how php works currently.

However there is also:

If a typed property is in uninitialized state, either because it has not yet been initialized, or because it has been explicitly unset(), then reads from this property will invoke the __get() method if it exists, consistently with the behavior of ordinary properties.

Currently this is not true. If property is "defined, uninitialized but not unset", Typed property Cl::$name must not be accessed before initialization error is shown. I personally like the current behaviour as __get magic is not invoked when property is defined but not initialized. The proposed ReflectionProperty::setUninitialized() reflection method does not impose any BC break and is consistent with ReflectionProperty::isInitialized() method used to check if a property is initialized or not.

@iluuu1994
Copy link
Member

iluuu1994 commented Aug 20, 2022

The __get return type validation againt typed property is mentioned in https://wiki.php.net/rfc/typed_properties_v2#overloaded_properties. This is also how php works currently.

Ah right. That makes sense.

Currently this is not true

Yes, but as you mentioned it was changed deliberately after the RFC has been voted on. f1848a4

IMO unsetting typed properties and keeping them unset sounds like a bad idea, as now any property access might succeed throw just one dubious method call later (with this behavior, that is). The inconsistency with __get is unfortunate, but IMO it would be better to disallow unsetting typed properties entirely.

So personally I'd rather see that gone, but others might disagree. According to f1848a4 this was only added as a workaround for ORMs anyway.

This gives us both the behavior people generally expect, and still allows ORMs to do lazy initialization by unsetting properties.

I don't quite understand why that would be needed though.

@mvorisek
Copy link
Contributor Author

One example why it is needed for ORM:

class defined by an user:

class CarEntity {
    public int $id;
    public string $name;
}

is extended (on runtime) like:

class CarEntity__ORMEntity extends CarEntity {
    trait ORMEntityTrait;
}

by ORM framework and evaluated (using eval or more often saved into some cache dir and included to be cacheable). The ORMEntityTrait trait contains magic __get, __set, ... methods defined.

This allows the CarEntity class to be used alone (/wo having to declare any ORM trait or extend any specific ORM class).

If ORM manager wants to use some magic ORM magic, it creates the CarEntity__ORMEntity object and unsets the defined properties to postpone their load (and other magic) until they are accessed. This is needed to satisfy OOP, as the lazy class needs to extend the simple entity class.

In atk4/data framework we have things called model and entity. They both share the same class, an entity instance is created by cloning model instance. Some entity properties are desired to defined once for all entities and parent model, thus we unset them in https://github.com/atk4/data/blob/c6bfad1acb588f850f93bc5773aede2a832e23c1/src/Model.php#L395.

Given both examples would be impossible when typed properties would not be possible to unset, I belive, and thanks got to f1848a4, all (mutable) properties must be allowed to be unset.

@mvorisek
Copy link
Contributor Author

One usecase/example of this issue - https://github.com/atk4/data/blob/5.0.0/src/Model/Scope.php#L67 - the Scope class has parent class with magic getter method. Is there currently any workaround to trully uninitialize the property without unsetting it?

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

2 participants