Fix class name handling in DeprecationHelper

The method for getting the declaring class name was not used when
printing the class name, and was incorrect anyway. Use reflection
when on the error path to ensure the correct class name is used.

Change-Id: Ic9cd4319535d5ab877a0563e0433371e1025d985

diff --git a/includes/debug/DeprecationHelper.php b/includes/debug/DeprecationHelper.php
index dc73ac9..0e1ee6b 100644 (file)
--- a/includes/debug/DeprecationHelper.php
+++ b/includes/debug/DeprecationHelper.php
@@ -79,8 +79,9 @@ trait DeprecationHelper {
                        return $this->$name;
                }
 
-               $qualifiedName = __CLASS__ . '::$' . $name;
-               if ( $this->deprecationHelperGetPropertyOwner( $name ) ) {
+               $ownerClass = $this->deprecationHelperGetPropertyOwner( $name );
+               $qualifiedName = ( $ownerClass ?: get_class( $this ) ) . '::$' . $name;
+               if ( $ownerClass ) {
                        // Someone tried to access a normal non-public property. Try to behave like PHP would.
                        trigger_error( "Cannot access non-public property $qualifiedName", E_USER_ERROR );
                } else {
@@ -99,8 +100,9 @@ trait DeprecationHelper {
                        return;
                }
 
-               $qualifiedName = __CLASS__ . '::$' . $name;
-               if ( $this->deprecationHelperGetPropertyOwner( $name ) ) {
+               $ownerClass = $this->deprecationHelperGetPropertyOwner( $name );
+               $qualifiedName = ( $ownerClass ?: get_class( $this ) ) . '::$' . $name;
+               if ( $ownerClass ) {
                        // Someone tried to access a normal non-public property. Try to behave like PHP would.
                        trigger_error( "Cannot access non-public property $qualifiedName", E_USER_ERROR );
                } else {
@@ -113,22 +115,12 @@ trait DeprecationHelper {
         * Like property_exists but also check for non-visible private properties and returns which
         * class in the inheritance chain declared the property.
         * @param string $property
-        * @return string|bool Best guess for the class in which the property is defined.
+        * @return string|bool Best guess for the class in which the property is defined. False if
+        *   the object does not have such a property.
         */
        private function deprecationHelperGetPropertyOwner( $property ) {
-               // Easy branch: check for protected property / private property of the current class.
-               if ( property_exists( $this, $property ) ) {
-                       // The class name is not necessarily correct here but getting the correct class
-                       // name would be expensive, this will work most of the time and getting it
-                       // wrong is not a big deal.
-                       return __CLASS__;
-               }
-               // property_exists() returns false when the property does exist but is private (and not
-               // defined by the current class, for some value of "current" that differs slightly
-               // between engines).
-               // Since PHP triggers an error on public access of non-public properties but happily
-               // allows public access to undefined properties, we need to detect this case as well.
-               // Reflection is slow so use array cast hack to check for that:
+               // Returning false is a non-error path and should avoid slow checks like reflection.
+               // Use array cast hack instead.
                $obfuscatedProps = array_keys( (array)$this );
                $obfuscatedPropTail = "\0$property";
                foreach ( $obfuscatedProps as $obfuscatedProp ) {
@@ -136,8 +128,9 @@ trait DeprecationHelper {
                        if ( strpos( $obfuscatedProp, $obfuscatedPropTail, 1 ) !== false ) {
                                $classname = substr( $obfuscatedProp, 1, -strlen( $obfuscatedPropTail ) );
                                if ( $classname === '*' ) {
-                                       // sanity; this shouldn't be possible as protected properties were handled earlier
-                                       $classname = __CLASS__;
+                                       // protected property; we didn't get the name, but we are on an error path
+                                       // now so it's fine to use reflection
+                                       return ( new ReflectionProperty( $this, $property ) )->getDeclaringClass()->getName();
                                }
                                return $classname;
                        }

diff --git a/tests/phpunit/includes/debug/DeprecationHelperTest.php b/tests/phpunit/includes/debug/DeprecationHelperTest.php
index 6b977a3..b14d89c 100644 (file)
--- a/tests/phpunit/includes/debug/DeprecationHelperTest.php
+++ b/tests/phpunit/includes/debug/DeprecationHelperTest.php
@@ -118,6 +118,16 @@ class DeprecationHelperTest extends MediaWikiTestCase {
                        $wrapper = TestingAccessWrapper::newFromObject( $this->testSubclass );
                        $this->assertSame( 1, $wrapper->privateNonDeprecated );
                }, E_USER_ERROR, "Cannot access non-public property $fullName" );
+
+               $fullName = 'TestDeprecatedSubclass::$subclassPrivateNondeprecated';
+               $this->assertErrorTriggered( function () {
+                       $this->assertSame( null, $this->testSubclass->subclassPrivateNondeprecated );
+               }, E_USER_ERROR, "Cannot access non-public property $fullName" );
+               $this->assertErrorTriggered( function () {
+                       $this->testSubclass->subclassPrivateNondeprecated = 0;
+                       $wrapper = TestingAccessWrapper::newFromObject( $this->testSubclass );
+                       $this->assertSame( 1, $wrapper->subclassPrivateNondeprecated );
+               }, E_USER_ERROR, "Cannot access non-public property $fullName" );
        }
 
        protected function assertErrorTriggered( callable $callback, $level, $message ) {

diff --git a/tests/phpunit/includes/debug/TestDeprecatedSubclass.php b/tests/phpunit/includes/debug/TestDeprecatedSubclass.php
index 0b6c8cf..28f8fa2 100644 (file)
--- a/tests/phpunit/includes/debug/TestDeprecatedSubclass.php
+++ b/tests/phpunit/includes/debug/TestDeprecatedSubclass.php
@@ -2,6 +2,8 @@
 
 class TestDeprecatedSubclass extends TestDeprecatedClass {
 
+       private $subclassPrivateNondeprecated = 1;
+
        public function getDeprecatedPrivateParentProperty() {
                return $this->privateDeprecated;
        }