From a015ee72aedd19df9b4647636d18130048e0f59c Mon Sep 17 00:00:00 2001 From: =?utf8?q?Gerg=C5=91=20Tisza?= Date: Fri, 19 Apr 2019 00:36:33 -0700 Subject: [PATCH] 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 --- includes/debug/DeprecationHelper.php | 33 ++++++++----------- .../includes/debug/DeprecationHelperTest.php | 10 ++++++ .../includes/debug/TestDeprecatedSubclass.php | 2 ++ 3 files changed, 25 insertions(+), 20 deletions(-) diff --git a/includes/debug/DeprecationHelper.php b/includes/debug/DeprecationHelper.php index dc73ac96fd..0e1ee6b023 100644 --- 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 6b977a3eaf..b14d89c3f4 100644 --- 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 0b6c8cf6f1..28f8fa24b2 100644 --- 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; } -- 2.20.1