ReflectionProperty inconsistently checks the instance type
Nobody has claimed this yet.
- Dominant language
- C
- Stars
- 40.4k
- Forks
- 8.1k
- Avg merge
- 2d 13h
- Merged PRs (30d)
- 96
Description
Description
Methods of ReflectionProperty that accept an object instance, are checking the object type inconsistently.
Given a ReflectionProperty created with new ReflectionProperty($className, $propName), here is a breakdown of the checks performed by the different methods:
getValue(),isInitialized():$objectmust be an instance of$classNameor of$propName's declaring class ($classNameitself or a parent)getRawValue(),setRawValueWithoutLazyInitialization():$objectmust be an instance of$classNamesetValue(),setRawValue():$objectmust be an object
Technically, these methods would work with any object, except setRawValueWithoutLazyInitialization which accesses the property backing store directly. However it makes sense that methods of new ReflectionProperty($className, $propName) don't accept instances unrelated to $className.
Historically, the check in getValue() was added as part of https://bugs.php.net/bug.php?id=72209, and only accepted $className. It was then changed to accept the declaring class in c97b8bbf8252, and both in 0e3045ae69d1, along with a TODO comment suggesting to accept only $className (but doing so would have been a BC). The check was propagated to getRawValue, setRawValueWithoutLazyInitialization with the TODO comment applied.
The inconsistency is not a huge issue, but it makes it impossible to cache the check result in https://github.com/php/php-src/pull/17698 (if the check were consistent, it would be possible to bypass the instanceof check when the cache is populated for the object's class).
Unfortunately, making the tests consistent would be a BC break:
- Changing to "
$objectmust be an instance of$classNameor of$propName's declaring class" would break https://3v4l.org/Tuej7 - Changing to "
$objectmust be an instance of$className" would break the same use-case, and also https://3v4l.org/E0srl
I'm not sure it's worth changing.
PHP Version
PHP-8.0
Operating System
No response
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by comparing the instance checks in ReflectionProperty's getValue(), isInitialized(), getRawValue(), setRawValueWithoutLazyInitialization(), setValue(), and setRawValue() methods. Review bug 72209, the referenced commits, and the linked examples before deciding whether a consistent rule is compatible; done means an agreed behavior with corresponding tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- php
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100