Fix GH-24147: Reflection name/class can be modified by compound assignment

[lazerg]

Plain assignment to $name/$class on reflection objects already throws from write_property, but compound assignment, ++/-- and &$r->name go through get_property_ptr_ptr, which reflection didn't override, so they wrote to the property slot directly. This adds a get_property_ptr_ptr handler that throws the same "Cannot set read-only property" exception for those two properties.

read_property also no longer caches the offset of these two properties. Otherwise $fn($r->class) called first by value and then by reference from the same call site reuses the cached offset and never reaches get_property_ptr_ptr.

Fixes #24147

[kamil-tekiela]

I don't think this is the right fix. IMHO, these properties should have been declared readonly, not special-cased like this. Is there a reason why we cannot fix the declaration?

[kamil-tekiela]

I don't think this is the right fix. IMHO, these properties should have been declared readonly, not special-cased like this. Is there a reason why we cannot fix the declaration?

Right, so we cannot do it exactly like that because of inheritance. PHP is sometimes so stupid.

• With this PR, foreach ($r as &$v) and unset() still break the assumption that the variable is read-only.
• With just readonly modifier, the value can be entirely different and it's not guaranteed that it won't change.
• With the existing write handler + readonly, the property becomes read-only, but the error message becomes inconsistent. Either Error or ReflectionException.
• With the existing handler + private(set) + Foreach by reference ignores property visibility #24280 fixed, the property becomes read-only, but the error message becomes inconsistent. Either Error or ReflectionException.

[ndossche]

don't think this is the right fix. IMHO, these properties should have been declared readonly, not special-cased like this. Is there a reason why we cannot fix the declaration?

The last time I looked into this (I'm not sure it's exactly this), changing the fake-readonly behaviour broke BetterReflection as it actually relies on being able to hack around these protections.

[kamil-tekiela]

don't think this is the right fix. IMHO, these properties should have been declared readonly, not special-cased like this. Is there a reason why we cannot fix the declaration?

The last time I looked into this (I'm not sure it's exactly this), changing the fake-readonly behaviour broke BetterReflection as it actually relies on being able to hack around these protections.

I saw that. But they are relying on an undefined-behaviour hack, and they should change their approach. It's only applicable to unset so we can skip this fix if they cannot change it (by trying something like this PR proposes). And we should probably fix this on master only.

[ndossche]

Definitely master-only, may be good to ping them as well.

[lazerg]

Makes sense, I rebased this onto master and added the note to UPGRADING. The PR still leaves unset() alone, so BetterReflection's unset($this->name) in their constructors keeps working as before. Happy to open an issue on their side too, unless one of you would rather ping them.