php / php/php-src

ReflectionProperty inconsistently checks the instance type

オープン
#17,730 コメント 1 件 リアクション 0 件 担当者 0 名 GitHub で見る

まだ誰も着手していません。

Bug Extension: reflection Status: Needs Triage
主要言語
C
スター
40.4k
フォーク
8.2k
平均マージ
2日 13時間
マージ済み PR(30日)
96

説明

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(): $object must be an instance of $className or of $propName's declaring class ($className itself or a parent)
  • getRawValue(), setRawValueWithoutLazyInitialization(): $object must be an instance of $className
  • setValue(), setRawValue(): $object must 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 "$object must be an instance of $className or of $propName's declaring class" would break https://3v4l.org/Tuej7
  • Changing to "$object must 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

コントリビューションガイド

コントリビューションガイドを開く

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

調査の方向性

まず、ReflectionProperty の getValue()、isInitialized()、getRawValue()、setRawValueWithoutLazyInitialization()、setValue()、setRawValue() メソッドにおけるインスタンスチェックを比較します。一貫したルールが適用可能かどうかを判断する前に、bug 72209、参照されているコミット、リンク先の例を確認してください。対応するテストを伴う合意済みの動作が定まれば完了です。

索引モデルが issue の本文から書いたものです。

評価

技術スタック
php
領域
backend
issue の種類
バグ
難易度
5/5
見積もり時間
1週間以上
活発さ
静か
明瞭さ
おおむね明確
初心者へのやさしさ
35/100

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。