Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Move is_subclass_of code to Zend\Stdlib #1807

Merged
merged 15 commits into from

7 participants

@prolic

@see https://bugs.php.net/bug.php?id=53727

This code is used to determine the subclass of a given class. Used in Zend\Di, Zend\Form,
Zend\InfoCard, Zend\Loader, Zend\Mvc, Zend\ServiceManager and Zend\XmlRpc

@prolic prolic Move is_subclass_of code to Zend\Stdlib
@see https://bugs.php.net/bug.php?id=53727

This code is used to determine the subclass of a given class. Used in Zend\Di, Zend\Form,
Zend\InfoCard, Zend\Loader, Zend\Mvc, Zend\ServiceManager and Zend\XmlRpc
9ebee1f
@travisbot

This pull request fails (merged 9ebee1f into 402074c).

@travisbot

This pull request fails (merged f374a41 into 402074c).

library/Zend/Stdlib/SubClass.php
((14 lines not shown))
+ * Checks if the object has this class as one of its parents
+ *
+ * @see https://bugs.php.net/bug.php?id=53727
+ *
+ * @param object|string $object
+ * @param string $type
+ */
+ public static function isSubclassOf($object, $type)
+ {
+ if (version_compare(PHP_VERSION, '5.3.7', '>=')) {
+ return is_subclass_of($object, $type);
+ }
+ if (is_object($object)) {
+ $object = get_class($object);
+ }
+ if (!array_key_exists($object, self::$cache)) {

Should that not be case-insensitive ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@prolic

Fixed, thanks @marc-mabe

@travisbot

This pull request fails (merged 1db1a1f into 402074c).

@marc-mabe

Why $object contains a name of a class-> IMHO $classname ?

@marc-mabe

Mh you should call class_parents & class_implements with the given classname because of autoloading will be done and could be non case-insensitive.

@prolic

Fixed, thanks @marc-mabe

@travisbot

This pull request fails (merged 789d837 into 402074c).

@travisbot

This pull request fails (merged fc00d87 into 402074c).

@travisbot

This pull request fails (merged 9dd39a5 into 402074c).

library/Zend/Stdlib/SubClass.php
((16 lines not shown))
+ * @category Zend
+ * @package Zend_Stdlib
+ * @subpackage SubClass
+ */
+abstract class SubClass
+{
+
+ /**
+ * Checks if the object has this class as one of its parents
+ *
+ * @see https://bugs.php.net/bug.php?id=53727
+ *
+ * @param object|string $object
+ * @param string $type
+ */
+ public static function isSubclassOf($object, $type)
@ralphschindler Collaborator

This implementation looks really expensive. I'd not accept objects here since PHP already handles objects just fine ($obj instanceof Type).

This abstraction should really only be for string class comparisons. Also, the body below can also be simplified for performance.

@prolic
prolic added a note

Hi @ralphschindler
PHP handles objects just fine with its instanceof operator. However is_subclass_of accepts also objects and the is_subclass_of function is also broken for objects.

see:

php5 -v

PHP 5.3.6-13ubuntu3.8 with Suhosin-Patch (cli) (built: Jun 13 2012 18:02:19)
Copyright (c) 1997-2011 The PHP Group
Zend Engine v2.3.0, Copyright (c) 1998-2011 Zend Technologies
with Xdebug v2.1.0, Copyright (c) 2002-2010, by Derick Rethans

php5 -a

interface A {}
class B implements A {}
class C extends B {}
var_dump(is_subclass_of('B', 'A'));
bool(false)
var_dump(is_subclass_of('C', 'A'));
bool(true)
var_dump(is_subclass_of(new B, 'A'));
bool(false)
var_dump(is_subclass_of(new C, 'A'));
bool(true)

About performance: The method is basically yours (taken from Zend\Di\Di), i just added the case-insensitive behaviour. I will look into this and see how this can be improved. Thanks for your review.

@ralphschindler Collaborator

Right, what I'm saying is that consuming code that wants to check if a particular object is of a particular type, it should just use instanceof, not is_subclass_of. is_subclass_of (php4) predates instanceof (php5) and its only usefulness is that it can be used with class name to class name comparisons.

As for the implementation, I'd simply remove the foreach. I'd create a multi dim array of $classes[$parent][$implements] = true; For the lower case stuff, I'd use array_change_case over ($classes[$parent]). What this does is remove the loop (expensive), and also keeps the lookup to an isset().

@prolic
prolic added a note

@ralphschindler I changed the implementation. It's now much faster. Tested various combinations, and this seems to be the fastet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Stdlib/SubClass.php
((28 lines not shown))
+ * @param object|string $object
+ * @param string $type
+ */
+ public static function isSubclassOf($object, $type)
+ {
+ if (version_compare(PHP_VERSION, '5.3.7', '>=')) {
+ return is_subclass_of($object, $type);
+ }
+ if (is_object($object)) {
+ $className = get_class($object);
+ } else {
+ $className = $object;
+ }
+ $className = ltrim($className, '\\');
+ $type = ltrim($type, '\\');
+ static $isSubclassFuncCache = null; // null as unset, array when set

Why not static $cache = array(); ?

@weierophinney Owner

Agree with @marc-mabe here. In fact, why not have the cache as a static protected member instead?

@ralphschindler Collaborator

The only reason it was a static variable before was to keep it out of the DI API, since we've created an API for this function, it should be fine to make it a static member of this class.

@prolic
prolic added a note

Implementation detail changed. No need for a cache now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Stdlib/SubClass.php
((18 lines not shown))
+ * @subpackage SubClass
+ */
+abstract class SubClass
+{
+
+ /**
+ * Checks if the object has this class as one of its parents
+ *
+ * @see https://bugs.php.net/bug.php?id=53727
+ *
+ * @param object|string $object
+ * @param string $type
+ */
+ public static function isSubclassOf($object, $type)
+ {
+ if (version_compare(PHP_VERSION, '5.3.7', '>=')) {

Not tested but this one should be much more performance:

if (is_subclass_of($object, $type)) {
return true;
}

if (version_compare(\PHP_VERSION, '5.3.7', '<')) {
return false;
}

// Workaround for PHP < 5.3.7
$class = $object;
if (is_object($object)) {
$class = get_class($object);
}

if (!class_exists($class, true)) {
return false;
}

$classNormalized = ltrim(strtolower($class), '\');
$typeNormalized = ltrim(strtolower($type), '\');

static $cache = array();
if (!isset($cache[$classNormalized])) {
$parents = class_parents($class) + class_implements($class);
$parents = array_map('strtolower', $parents);
$cache[$classNormalized] = $parents;
}

return in_array($typeNormalized, $cache[$classNormalized]);

Sry it should be ... if (version_compare(\PHP_VERSION, '5.3.7', '>=')) { ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Stdlib/SubClass.php
((22 lines not shown))
+
+ /**
+ * Checks if the object has this class as one of its parents
+ *
+ * @see https://bugs.php.net/bug.php?id=53727
+ *
+ * @param object|string $object
+ * @param string $type
+ */
+ public static function isSubclassOf($object, $type)
+ {
+ if (version_compare(PHP_VERSION, '5.3.7', '>=')) {
+ return is_subclass_of($object, $type);
+ }
+ if (is_object($object)) {
+ $className = get_class($object);
@Maks3w Collaborator
Maks3w added a note

This can be replaced by: return ($object instanceof $type)

@prolic
prolic added a note

fixed, thanks

@ralphschindler Collaborator

As I said, we don't need to incur the cost of a function call where instanceof would have sufficed. This method should only deal with $class, $type comparisons, not $object $type comparisons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@marc-mabe marc-mabe commented on the diff
library/Zend/Stdlib/SubClass.php
((5 lines not shown))
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2012 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ * @package Zend_Stdlib
+ */
+
+namespace Zend\Stdlib;
+
+/**
+ * @see https://bugs.php.net/bug.php?id=53727
+ *
+ * @category Zend
+ * @package Zend_Stdlib
+ * @subpackage SubClass
+ */
+abstract class SubClass

Name it ClassUtils or OopUtils ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Stdlib/SubClass.php
((32 lines not shown))
+ {
+ if (version_compare(PHP_VERSION, '5.3.7', '>=')) {
+ return is_subclass_of($object, $type);
+ }
+ if (is_object($object)) {
+ $className = get_class($object);
+ } else {
+ $className = $object;
+ }
+ $className = ltrim($className, '\\');
+ $type = ltrim($type, '\\');
+ static $isSubclassFuncCache = null; // null as unset, array when set
+ if ($isSubclassFuncCache === null) {
+ $isSubclassFuncCache = array();
+ }
+ if (!array_key_exists($className, $isSubclassFuncCache)) {
@weierophinney Owner

First, you store the value of strtolower($className) -- as such, this condition will always fail.

Instead of doing the work inside the conditional, have a conditional test if the array key exists, and return the value. Then do the work in the main body.

$normalizedClassName = strtolower($className);
$normalizedType             = strtolower($type);
if (array_key_exists($normalizedClassName, $isSubclassFuncCache)) {
    return isset($isSubclassFuncCache[$normalizedClassName][$normalizedType];
}
$parents = class_parents($className, true) + class_implements($className, true);
/* ... */
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/Zend/Stdlib/SubClassTest.php
((4 lines not shown))
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2012 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ * @package Zend_Stdlib
+ */
+
+namespace ZendTest\Stdlib;
+
+use Zend\Stdlib\SubClass;
+
+class SubClassTest extends \PHPUnit_Framework_TestCase
+{
+ protected function setUp()
+ {
+ if (version_compare(PHP_VERSION, '5.3.7', '>=')) {
@Maks3w Collaborator
Maks3w added a note

I think that is best test it with all versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@prolic

I changed the implementation. It's now much faster. About the class name: Why not Zend\Stdlib\PhpBugs or something like that? I guess we can reimplement some php-functions, that are broken.

@travisbot

This pull request fails (merged 99dca8e into 402074c).

@travisbot

This pull request fails (merged 7b2f30e into 402074c).

@travisbot

This pull request fails (merged a14f2f7 into 402074c).

@prolic

If someone finds a faster solution, please send it to me! :-) Perhabs I should rebase, so we merge not so many commits, just a single one.

@ralphschindler
Collaborator

For the record, I am generally against factoring this out of DI. Every line of code in DI extremely deliberate and designed to be as fast as possible, especially given the problem it is solving. The implementation of that particular function is suited for that particular hack (you'll notice I don't do any case switching since the likelihood of case being an issue is next to nil, so there is no need to incur that cost).

@prolic

@ralphschindler can you please test your implementation against my current on your machine? On my machine my current implementation using Reflection is faster.

@travisbot

This pull request fails (merged 1661628 into 402074c).

@travisbot

This pull request fails (merged e89cdcf into 402074c).

@ralphschindler ralphschindler commented on the diff
library/Zend/Stdlib/SubClass.php
((30 lines not shown))
+ * @param string $className
+ * @param string $type
+ */
+ public static function isSubclassOf($className, $type)
+ {
+ if (version_compare(PHP_VERSION, '5.3.7', '>=')) {
+ return is_subclass_of($className, $type);
+ }
+ if (is_subclass_of($className, $type)) {
+ return true;
+ }
+ if (!interface_exists($type)) {
+ return false;
+ }
+ $r = new ReflectionClass($className);
+ return $r->implementsInterface($type);
@ralphschindler Collaborator

Is using Reflection here not jumping out as a bad idea? Would you expect that by calling this seemingly innocuous method (SubClass::isSubclassOf()) would go and instantiate a reflection object to answer the question?
Also, instead of implementsInterface, I think you were looking for $r->isSubclassOf().

@prolic
prolic added a note

Well it's faster, so why not use it.
I am using implementsInterface, because I can be sure, that $type is an interface. Otherwise the method would return earlier.

@weierophinney Owner

Did you test this against a version that caches? Otherwise, every time you test a given class, it will have to do the reflection, which is going to be slow.

Honestly, I'm not 100% convinced we should abstract this into a static method call; the handful of places it was used before were each optimized for the specific use cases, and you'll notice they differ in approach based on context.

@prolic
prolic added a note

I will put performance tests online, so everybody can compare the speed difference.
Generally I like that we have a central place, where we care about this php bug. Also we provide a zf-way for end-users, so they know how to handle the situation with this specific php bug. They can just use a static method call until 5.4 is the minimum php version and the class is removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@prolic

I can remove the dependency in Zend\Di\Di again, if that is wished. Any other thoughts on this except @ralphschindler who brought this up?

@prolic

performance comparisions online:
https://gist.github.com/3089749

@weierophinney

Performance results look good. However, they are not testing the same implementations. Ideally, we should be testing the difference between calling a local method and calling a static method on another class. If I have time today, I'll fork your gist and see what results that presents.

@prolic

Great, but I don't expect that it will be then faster then with reflection :-)

@weierophinney

My experiments bear out your results: https://gist.github.com/3092671

Basically, the approach you propose is 3-4x faster than the original code in Di, and caching actually halves the gains. I'm convinced.

@weierophinney weierophinney merged commit f25e711 into from
@ralphschindler
Collaborator

So, I generally agree caching is no good in any instance. But your tests are flawed, and so are your numbers. In general, for DI's purposes, I don't care about case since all lookups contain valid classes already (the source of all types is Reflection in the first place). Beyond that, and this is as much a guess as your numbers, 3-5 times the amount of lookups will be for completely unrelated code in the first place - so that needs to be factored in.

https://gist.github.com/3092974

@weierophinney

Revised benchmarks, combining those of @prolic, myself, and @ralphschindler: https://gist.github.com/3092671

Basically, the uncached reflection implementation is on par with the fastest version @ralphschindler used (which uses class_parents() + class_implements() to do its work), but has the benefit of being case sensitive -- which is useful for DI use cases.

However, I'm unconvinced that we should have this as a centralized solution. In particular, 4 of the 8 components did not previously have a dep on Stdlib, and in the case of the AutoloaderFactory, there should be no dependencies outside of the autoloaders themselves.

@weierophinney weierophinney referenced this pull request from a commit in weierophinney/zf2
@weierophinney weierophinney Remove Zend\Stdlib\SubClass
The main reason for this class was to work around changes in PHP between
versions 5.3.3 and 5.3.7 regarding how is_subclass_of() works. In the earlier
versions, it was buggy. In 5.3.7 and up, the bugs are fixed, and the solution is
very fast.

There are very few places in the framework where an is_subclass_of() test makes
sense, and those are typically only with classes responsible for creating other
classes -- factories, primarily. As such, the number of places where this will
be of use is minimal, and can be maintained.

The main goal is to have a consistent, performant way to do the check when
required, and one which will work with case sensitive contexts. The solution by
@prolic in #1807 is good; we just don't need to abstract this, particularly as
we'll be able to remove this entirely in ZF3. Additionally, abstracting it gives
additional dependencies for those components needing it, and in cases like
autoloaders, DI, and the service manager, the extra dependency is undesirable as
these should stand alone.

The code has been duplicated into each class requiring the lookup (I put the
check into AbstractPluginManager, as it's not unlikely other plugin managers may
need this).
387c529
@weierophinney

New PR about this: #1852

@EvanDotPro
Collaborator

While I generally advocate that cross-component dependencies are not evil, I have to say I agree at least in the case of the AutoloaderFactory -- this class should absolutely not have dependencies outside of Zend\Loader.

@EvanDotPro EvanDotPro referenced this pull request from a commit
@EvanDotPro EvanDotPro Remove require_once() in AutoloaderFactory
This dependency was removed by @weierophinney in PR #1852, but the
require_once was not removed. Originally the dependency was introduced
by @prolic in PR #1807.
3eb61e3
@ghost Unknown referenced this pull request from a commit
@weierophinney weierophinney Remove Zend\Stdlib\SubClass
The main reason for this class was to work around changes in PHP between
versions 5.3.3 and 5.3.7 regarding how is_subclass_of() works. In the earlier
versions, it was buggy. In 5.3.7 and up, the bugs are fixed, and the solution is
very fast.

There are very few places in the framework where an is_subclass_of() test makes
sense, and those are typically only with classes responsible for creating other
classes -- factories, primarily. As such, the number of places where this will
be of use is minimal, and can be maintained.

The main goal is to have a consistent, performant way to do the check when
required, and one which will work with case sensitive contexts. The solution by
@prolic in #1807 is good; we just don't need to abstract this, particularly as
we'll be able to remove this entirely in ZF3. Additionally, abstracting it gives
additional dependencies for those components needing it, and in cases like
autoloaders, DI, and the service manager, the extra dependency is undesirable as
these should stand alone.

The code has been duplicated into each class requiring the lookup (I put the
check into AbstractPluginManager, as it's not unlikely other plugin managers may
need this).
3894420
@ghost Unknown referenced this pull request from a commit
@EvanDotPro EvanDotPro Remove require_once() in AutoloaderFactory
This dependency was removed by @weierophinney in PR #1852, but the
require_once was not removed. Originally the dependency was introduced
by @prolic in PR #1807.
f0ff1de
@Ocramius Ocramius referenced this pull request from a commit
@Ocramius Ocramius #6785 - using `is_subclass_of()` instead of protected static method u…
…sage

See bug https://bugs.php.net/bug.php?id=53727
See bug #1807

Deprecating protected static polyfill method `isSubclassOf`
992c187
@Ocramius Ocramius referenced this pull request from a commit
@Ocramius Ocramius #6785 - using `is_subclass_of()` instead of protected static method u…
…sage

See bug https://bugs.php.net/bug.php?id=53727
See bug #1807

Deprecating protected static polyfill method `isSubclassOf`
8c73319
@Ocramius Ocramius referenced this pull request from a commit
@Ocramius Ocramius #6785 - using `is_subclass_of()` instead of protected static method u…
…sage

See bug https://bugs.php.net/bug.php?id=53727
See bug #1807

Deprecating protected static polyfill method `isSubclassOf`
dc6367b
@Ocramius Ocramius referenced this pull request from a commit
@Ocramius Ocramius #6785 - using `is_subclass_of()` instead of protected static method u…
…sage

See bug https://bugs.php.net/bug.php?id=53727
See bug #1807

Deprecating protected static polyfill method `isSubclassOf`
e25d373
@Ocramius Ocramius referenced this pull request from a commit
@Ocramius Ocramius #6785 - using `is_subclass_of()` instead of protected static method u…
…sage

See bug https://bugs.php.net/bug.php?id=53727
See bug #1807

Deprecating protected static polyfill method `isSubclassOf`
fcadad3
@fabiocarneiro fabiocarneiro referenced this pull request from a commit in fabiocarneiro/zend-form
@Ocramius Ocramius #6785 - using `is_subclass_of()` instead of protected static method u…
…sage

See bug https://bugs.php.net/bug.php?id=53727
See bug zendframework/zf2#1807

Deprecating protected static polyfill method `isSubclassOf`
618639b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 9, 2012
  1. @prolic

    Move is_subclass_of code to Zend\Stdlib

    prolic authored
    @see https://bugs.php.net/bug.php?id=53727
    
    This code is used to determine the subclass of a given class. Used in Zend\Di, Zend\Form,
    Zend\InfoCard, Zend\Loader, Zend\Mvc, Zend\ServiceManager and Zend\XmlRpc
  2. @prolic

    Fix autoloader factory

    prolic authored
  3. @prolic
  4. @prolic
  5. @prolic

    fixed isSubclassOf method

    prolic authored
  6. @prolic
Commits on Jul 10, 2012
  1. @prolic

    added tests, added file header

    prolic authored
  2. @prolic
  3. @prolic
  4. @prolic
  5. @prolic

    small fix

    prolic authored
  6. @prolic

    changed class_exists with interface_exists

    prolic authored
    sorry, seams I need more sleep :-)
  7. @prolic
  8. @prolic
  9. @prolic
This page is out of date. Refresh to see the latest.
View
29 library/Zend/Di/Di.php
@@ -3,6 +3,7 @@
namespace Zend\Di;
use Closure;
+use Zend\Stdlib\SubClass;
class Di implements DependencyInjectionInterface
{
@@ -301,7 +302,7 @@ protected function handleInjectDependencies($instance, $injectionMethods, $param
if ($methodParams) {
foreach ($methodParams as $methodParam) {
$objectToInjectClass = $this->getClass($objectToInject);
- if ($objectToInjectClass == $methodParam[1] || $this->isSubclassOf($objectToInjectClass, $methodParam[1])) {
+ if ($objectToInjectClass == $methodParam[1] || SubClass::isSubclassOf($objectToInjectClass, $methodParam[1])) {
if ($this->resolveAndCallInjectionMethodForInstance($instance, $typeInjectionMethod, array($methodParam[0] => $objectToInject), $instanceAlias, true, $type)) {
$calledMethods[$typeInjectionMethod] = true;
}
@@ -574,7 +575,7 @@ protected function resolveMethodParameters($class, $method, array $callTimeUserP
}
$pInstanceClass = ($this->instanceManager->hasAlias($pInstance)) ?
$this->instanceManager->getClassFromAlias($pInstance) : $pInstance;
- if ($pInstanceClass === $type || $this->isSubclassOf($pInstanceClass, $type)) {
+ if ($pInstanceClass === $type || SubClass::isSubclassOf($pInstanceClass, $type)) {
$computedParams['required'][$fqParamPos] = array($pInstance, $pInstanceClass);
continue 2;
}
@@ -591,7 +592,7 @@ protected function resolveMethodParameters($class, $method, array $callTimeUserP
}
$pInstanceClass = ($this->instanceManager->hasAlias($pInstance)) ?
$this->instanceManager->getClassFromAlias($pInstance) : $pInstance;
- if ($pInstanceClass === $type || $this->isSubclassOf($pInstanceClass, $type)) {
+ if ($pInstanceClass === $type || SubClass::isSubclassOf($pInstanceClass, $type)) {
$computedParams['required'][$fqParamPos] = array($pInstance, $pInstanceClass);
continue 2;
}
@@ -673,26 +674,4 @@ protected function getClass($instance)
return get_class($instance);
}
- /**
- * @see https://bugs.php.net/bug.php?id=53727
- *
- * @param $class
- * @param $type
- * @return bool
- */
- protected function isSubclassOf($class, $type)
- {
- /* @var $isSubclassFunc Closure */
- static $isSubclassFuncCache = null; // null as unset, array when set
-
- if ($isSubclassFuncCache === null) {
- $isSubclassFuncCache = array();
- }
-
- if (!array_key_exists($class, $isSubclassFuncCache)) {
- $isSubclassFuncCache[$class] = class_parents($class, true) + class_implements($class, true);
- }
- return (isset($isSubclassFuncCache[$class][$type]));
- }
-
}
View
22 library/Zend/Form/Annotation/AnnotationBuilder.php
@@ -33,6 +33,7 @@
use Zend\Form\Exception;
use Zend\Form\Factory;
use Zend\Stdlib\ArrayUtils;
+use Zend\Stdlib\SubClass;
/**
* Parses a class' properties for annotations in order to create a form and
@@ -339,7 +340,7 @@ protected function configureElement($annotations, $reflection, $formSpec, $filte
: 'Zend\Form\Element';
// Compose as a fieldset or an element, based on specification type
- if ($this->isFieldset($type)) {
+ if (SubClass::isSubclassOf($type, 'Zend\Form\FieldsetInterface')) {
if (!isset($formSpec['fieldsets'])) {
$formSpec['fieldsets'] = array();
}
@@ -386,23 +387,4 @@ protected function checkForExclude($annotations)
return (bool) $results->last();
}
- /**
- * Determine if the type represents a fieldset
- *
- * For PHP versions >= 5.3.7, uses is_subclass_of; otherwise, uses
- * reflection to determine the interfaces implemented.
- *
- * @param string $type
- * @return bool
- */
- protected function isFieldset($type)
- {
- if (version_compare(PHP_VERSION, '5.3.7', 'gte')) {
- return is_subclass_of($type, 'Zend\Form\FieldsetInterface');
- }
-
- $r = new ClassReflection($type);
- $interfaces = $r->getInterfaceNames();
- return (in_array('Zend\Form\FieldsetInterface', $interfaces));
- }
}
View
22 library/Zend/Form/Factory.php
@@ -27,6 +27,7 @@
use Zend\InputFilter\InputFilterInterface;
use Zend\Stdlib\ArrayUtils;
use Zend\Stdlib\Hydrator;
+use Zend\Stdlib\SubClass;
/**
* @category Zend
@@ -84,33 +85,20 @@ public function create($spec)
$spec = $this->validateSpecification($spec, __METHOD__);
$type = isset($spec['type']) ? $spec['type'] : 'Zend\Form\Element';
- if ($type instanceof FormInterface) {
+ if (SubClass::isSubclassOf($type, 'Zend\Form\FormInterface')) {
return $this->createForm($spec);
}
- if ($type instanceof FieldsetInterface) {
+ if (SubClass::isSubclassOf($type, 'Zend\Form\FieldsetInterface')) {
return $this->createFieldset($spec);
}
- if ($type instanceof ElementInterface) {
+ if (SubClass::isSubclassOf($type, 'Zend\Form\ElementInterface')) {
return $this->createElement($spec);
}
- if (is_string($type) && class_exists($type)) {
- $reflection = new ReflectionClass($type);
- if ($reflection->implementsInterface('Zend\Form\FormInterface')) {
- return $this->createForm($spec);
- }
- if ($reflection->implementsInterface('Zend\Form\FieldsetInterface')) {
- return $this->createFieldset($spec);
- }
- if ($reflection->implementsInterface('Zend\Form\ElementInterface')) {
- return $this->createElement($spec);
- }
- }
-
throw new Exception\DomainException(sprintf(
- '%s expects the $spec["type"] to implement one of %s, %s, %s, or a valid full qualified class name; received %s',
+ '%s expects the $spec["type"] to implement one of %s, %s, or %s; received %s',
__METHOD__,
'Zend\Form\ElementInterface',
'Zend\Form\FieldsetInterface',
View
3  library/Zend/InfoCard/XML/AbstractElement.php
@@ -23,6 +23,7 @@
use DOMElement;
use SimpleXMLElement;
+use Zend\Stdlib\SubClass;
/**
* An abstract class representing a an XML data block
@@ -80,7 +81,7 @@ static public function convertToObject(\DOMElement $e, $classname)
throw new Exception\InvalidArgumentException('Class provided for converting does not exist');
}
- if(!is_subclass_of($classname, 'Zend\InfoCard\XML\ElementInterface')) {
+ if (!SubClass::isSubclassOf($classname, 'Zend\InfoCard\XML\ElementInterface')) {
throw new Exception\InvalidArgumentException("DOM element must be converted to an instance of Zend_InfoCard_Xml_Element");
}
View
15 library/Zend/Loader/AutoloaderFactory.php
@@ -102,15 +102,12 @@ public static function factory($options = null)
);
}
- // unfortunately is_subclass_of is broken on some 5.3 versions
- // additionally instanceof is also broken for this use case
- if (version_compare(PHP_VERSION, '5.3.7', '>=')) {
- if (!is_subclass_of($class, 'Zend\Loader\SplAutoloader')) {
- require_once 'Exception/InvalidArgumentException.php';
- throw new Exception\InvalidArgumentException(
- sprintf('Autoloader class %s must implement Zend\\Loader\\SplAutoloader', $class)
- );
- }
+ require_once __DIR__ . '/../Stdlib/SubClass.php';
+ if (!\Zend\Stdlib\SubClass::isSubclassOf($class, 'Zend\Loader\SplAutoloader')) {
+ require_once 'Exception/InvalidArgumentException.php';
+ throw new Exception\InvalidArgumentException(
+ sprintf('Autoloader class %s must implement Zend\\Loader\\SplAutoloader', $class)
+ );
}
if ($class === static::STANDARD_AUTOLOADER) {
View
3  library/Zend/Mvc/Router/RoutePluginManager.php
@@ -21,6 +21,7 @@
namespace Zend\Mvc\Router;
+use Zend\Stdlib\SubClass;
use Zend\ServiceManager\AbstractPluginManager;
/**
@@ -92,7 +93,7 @@ protected function createFromInvokable($canonicalName, $requestedName)
));
}
- if (!$this->isSubclassOf($invokable, __NAMESPACE__ . '\RouteInterface')) {
+ if (!SubClass::isSubclassOf($invokable, __NAMESPACE__ . '\RouteInterface')) {
throw new Exception\RuntimeException(sprintf(
'%s: failed retrieving "%s%s" via invokable class "%s"; class does not implement %s\RouteInterface',
__METHOD__,
View
22 library/Zend/ServiceManager/AbstractPluginManager.php
@@ -20,8 +20,6 @@
namespace Zend\ServiceManager;
-use Zend\Code\Reflection\ClassReflection;
-
/**
* ServiceManager implementation for managing plugins
*
@@ -194,24 +192,4 @@ protected function createFromInvokable($canonicalName, $requestedName)
return $instance;
}
- /**
- * Determine if a class implements a given interface
- *
- * For PHP versions >= 5.3.7, uses is_subclass_of; otherwise, uses
- * reflection to determine the interfaces implemented.
- *
- * @param string $class
- * @param string $type
- * @return bool
- */
- protected function isSubclassOf($class, $type)
- {
- if (version_compare(PHP_VERSION, '5.3.7', 'gte')) {
- return is_subclass_of($class, $type);
- }
-
- $r = new ClassReflection($class);
- $interfaces = $r->getInterfaceNames();
- return (in_array($type, $interfaces));
- }
}
View
47 library/Zend/Stdlib/SubClass.php
@@ -0,0 +1,47 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2012 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ * @package Zend_Stdlib
+ */
+
+namespace Zend\Stdlib;
+
+use ReflectionClass;
+
+/**
+ * @see https://bugs.php.net/bug.php?id=53727
+ *
+ * @category Zend
+ * @package Zend_Stdlib
+ * @subpackage SubClass
+ */
+abstract class SubClass

Name it ClassUtils or OopUtils ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+{
+
+ /**
+ * Checks if the object has this class as one of its parents
+ *
+ * @see https://bugs.php.net/bug.php?id=53727
+ *
+ * @param string $className
+ * @param string $type
+ */
+ public static function isSubclassOf($className, $type)
+ {
+ if (version_compare(PHP_VERSION, '5.3.7', '>=')) {
+ return is_subclass_of($className, $type);
+ }
+ if (is_subclass_of($className, $type)) {
+ return true;
+ }
+ if (!interface_exists($type)) {
+ return false;
+ }
+ $r = new ReflectionClass($className);
+ return $r->implementsInterface($type);
@ralphschindler Collaborator

Is using Reflection here not jumping out as a bad idea? Would you expect that by calling this seemingly innocuous method (SubClass::isSubclassOf()) would go and instantiate a reflection object to answer the question?
Also, instead of implementsInterface, I think you were looking for $r->isSubclassOf().

@prolic
prolic added a note

Well it's faster, so why not use it.
I am using implementsInterface, because I can be sure, that $type is an interface. Otherwise the method would return earlier.

@weierophinney Owner

Did you test this against a version that caches? Otherwise, every time you test a given class, it will have to do the reflection, which is going to be slow.

Honestly, I'm not 100% convinced we should abstract this into a static method call; the handful of places it was used before were each optimized for the specific use cases, and you'll notice they differ in approach based on context.

@prolic
prolic added a note

I will put performance tests online, so everybody can compare the speed difference.
Generally I like that we have a central place, where we care about this php bug. Also we provide a zf-way for end-users, so they know how to handle the situation with this specific php bug. They can just use a static method call until 5.4 is the minimum php version and the class is removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ }
+}
View
7 library/Zend/XmlRpc/Server.php
@@ -21,10 +21,10 @@
namespace Zend\XmlRpc;
-use ReflectionClass;
use Zend\Server\AbstractServer;
use Zend\Server\Definition;
use Zend\Server\Reflection;
+use Zend\Stdlib\SubClass;
/**
* An XML-RPC server implementation
@@ -452,10 +452,9 @@ public function getResponse()
*/
public function setResponseClass($class)
{
- if (!class_exists($class) or
- ($c = new ReflectionClass($class) and !$c->isSubclassOf('Zend\\XmlRpc\\Response'))) {
-
+ if (!class_exists($class) || !SubClass::isSubclassOf($class, 'Zend\XmlRpc\Response')) {
throw new Server\Exception\InvalidArgumentException('Invalid response class');
+
}
$this->responseClass = $class;
return true;
View
30 tests/Zend/Stdlib/SubClassTest.php
@@ -0,0 +1,30 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2012 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ * @package Zend_Stdlib
+ */
+
+namespace ZendTest\Stdlib;
+
+use Zend\Stdlib\SubClass;
+
+class SubClassTest extends \PHPUnit_Framework_TestCase
+{
+ protected function setUp()
+ {
+ require_once 'TestAsset/DummySubclasses.php';
+ }
+
+ public function testIsSubclassOf()
+ {
+ $test1 = SubClass::isSubclassOf('ZendTest\Stdlib\TestAsset\ChildClass', 'ZendTest\Stdlib\TestAsset\TestInterface');
+ $test2 = SubClass::isSubclassOf('ZendTest\Stdlib\TestAsset\ParentClass', 'ZendTest\Stdlib\TestAsset\TestInterface');
+ $this->assertTrue($test1);
+ $this->assertTrue($test2);
+ }
+
+}
View
16 tests/Zend/Stdlib/TestAsset/DummySubclasses.php
@@ -0,0 +1,16 @@
+<?php
+
+namespace ZendTest\Stdlib\TestAsset
+{
+ interface TestInterface
+ {
+ }
+
+ class ParentClass implements TestInterface
+ {
+ }
+
+ class ChildClass extends ParentClass
+ {
+ }
+}
Something went wrong with that request. Please try again.