php / php/php-src

ReflectionProperty inconsistently checks the instance type

Ouverte
#17,730 1 commentaire 0 réactions 0 personnes assignées Voir sur GitHub

Personne n'a encore pris cette issue.

Bug Extension: reflection Status: Needs Triage
Langage dominant
C
Étoiles
40.4k
Forks
8.1k
Merge moyen
2 j 13 h
PR mergées (30 j)
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(): $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

Guide de contribution

Ouvrir le guide de contribution

Par où commencer

  1. Lisez l'issue en entier, puis le guide de contribution du projet.
  2. Signalez en commentaire que vous la prenez — cela évite que deux personnes fassent le même travail.
  3. Forkez le dépôt et travaillez sur une branche.
  4. Ouvrez une pull request qui référence le numéro de l'issue.

Piste de recherche

Commencez par comparer les vérifications d'instance dans les méthodes getValue(), isInitialized(), getRawValue(), setRawValueWithoutLazyInitialization(), setValue() et setRawValue() de ReflectionProperty. Examinez le bug 72209, les commits référencés et les exemples liés avant de décider si une règle cohérente est compatible ; le travail est terminé lorsqu'un comportement convenu avec les tests correspondants est défini.

Rédigé par le modèle d'indexation à partir du texte de l'issue.

Évaluation

Stack technique
php
Domaine
backend
Type d'issue
Bug
Difficulté
5/5
Temps estimé
Plus d'une semaine
Activité
Calme
Clarté
Plutôt claire
Accessibilité débutants
35/100

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.