Merge pull request #94 from marc-mabe/opt_Enum_detectConstnats

Optimize Enum::detectConstants() by using assertion

Author: marc-mabe

.travis.yml

@@ -49,9 +49,9 @@ install:
 
 script:
   - if [ "$CODE_COVERAGE" == "1" ]; then
-      php vendor/bin/phpunit --verbose --coverage-text --coverage-clover=coverage.clover;
+      php -d 'zend.assertions=1' vendor/bin/phpunit --verbose --coverage-text --coverage-clover=coverage.clover;
     else
-      php vendor/bin/phpunit --verbose;
+      php -d 'zend.assertions=1' vendor/bin/phpunit --verbose;
     fi
 
 after_script:

bench/EnumBench.php

@@ -2,7 +2,10 @@
 
 namespace MabeEnumBench;
 
+use MabeEnum\Enum;
 use MabeEnumTest\TestAsset\Enum66;
+use ReflectionClass;
+use ReflectionProperty;
 
 /**
  * Benchmark of abstract class Enum tested with enumeration of 66 enumerators.
@@ -17,6 +20,10 @@
  */
 class EnumBench
 {
+    /**
+     * @var ReflectionProperty[]
+     */
+    private $enumPropsRefl;
     /**
      * @var string[]
      */
@@ -42,12 +49,25 @@ class EnumBench
      */
     public function init()
     {
+        $enumRefl = new ReflectionClass(Enum::class);
+        $this->enumPropsRefl = $enumRefl->getProperties(ReflectionProperty::IS_STATIC);
+        foreach ($this->enumPropsRefl as $enumPropRefl) {
+            $enumPropRefl->setAccessible(true);
+        }
+
         $this->names       = Enum66::getNames();
         $this->values      = Enum66::getValues();
         $this->ordinals    = Enum66::getOrdinals();
         $this->enumerators = Enum66::getEnumerators();
     }
 
+    private function resetStaticEnumProps()
+    {
+        foreach ($this->enumPropsRefl as $enumPropRefl) {
+            $enumPropRefl->setValue([]);
+        }
+    }
+
     public function benchGetName()
     {
         foreach ($this->enumerators as $enumerator) {
@@ -85,6 +105,7 @@ public function benchIsByValue()
 
     public function benchGetConstants()
     {
+        $this->resetStaticEnumProps();
         Enum66::getConstants();
     }
 

bench/bootstrap.php

@@ -0,0 +1,14 @@
+<?php
+
+$zendassertions = ini_get('zend.assertions');
+if (\PHP_VERSION_ID >= 70000 && $zendassertions != -1) {
+    echo 'Please disable zend.assertions in php.ini (zend.assertions = -1)' . PHP_EOL
+        . "Current ini setting: zend.assertions = {$zendassertions}]" . PHP_EOL;
+    exit(1);
+}
+assert_options(ASSERT_ACTIVE, 0);
+assert_options(ASSERT_WARNING, 0);
+assert_options(ASSERT_BAIL, 0);
+assert_options(ASSERT_QUIET_EVAL, 0);
+
+require_once __DIR__ . '/../vendor/autoload.php';

phpbench.json

@@ -1,5 +1,5 @@
 {
-  "bootstrap": "vendor/autoload.php",
+  "bootstrap": "bench/bootstrap.php",
   "path": "bench/",
   "retry_threshold": 5
 }

phpunit.xml.dist

@@ -21,4 +21,8 @@
             <directory suffix=".php">./src</directory>
         </whitelist>
     </filter>
+    <php>
+        <ini name="zend.assertions" value="1"/>
+        <ini name="assert.exception" value="1"/>
+    </php>
 </phpunit>

src/Enum.php

@@ -2,6 +2,7 @@
 
 namespace MabeEnum;
 
+use AssertionError;
 use ReflectionClass;
 use InvalidArgumentException;
 use LogicException;
@@ -337,17 +338,17 @@ final public static function has($value)
      *
      * @param string $class
      * @return array
-     * @throws LogicException On ambiguous constant values
+     * @throws AssertionError On ambiguous constant values
      */
     private static function detectConstants($class)
     {
         if (!isset(self::$constants[$class])) {
             $reflection = new ReflectionClass($class);
-            $constants  = [];
+            $publicConstants  = [];
 
             do {
                 $scopeConstants = [];
-                if (PHP_VERSION_ID >= 70100) {
+                if (\PHP_VERSION_ID >= 70100) {
                     // Since PHP-7.1 visibility modifiers are allowed for class constants
                     // for enumerations we are only interested in public once.
                     foreach ($reflection->getReflectionConstants() as $reflConstant) {
@@ -360,33 +361,35 @@ private static function detectConstants($class)
                     $scopeConstants = $reflection->getConstants();
                 }
 
-                $constants = $scopeConstants + $constants;
+                $publicConstants = $scopeConstants + $publicConstants;
             } while (($reflection = $reflection->getParentClass()) && $reflection->name !== __CLASS__);
 
-            // Detect ambiguous values and report names
-            $ambiguous = [];
-            foreach ($constants as $value) {
-                $names = \array_keys($constants, $value, true);
-                if (\count($names) > 1) {
-                    $ambiguous[\var_export($value, true)] = $names;
-                }
-            }
-            if (!empty($ambiguous)) {
-                throw new LogicException(
-                    'All possible values needs to be unique. The following are ambiguous: '
-                    . \implode(', ', \array_map(function ($names) use ($constants) {
-                        return \implode('/', $names) . '=' . \var_export($constants[$names[0]], true);
-                    }, $ambiguous))
-                );
-            }
+            assert(self::noAmbiguousValues($publicConstants));
 
-            self::$constants[$class] = $constants;
-            self::$names[$class] = \array_keys($constants);
+            self::$constants[$class] = $publicConstants;
+            self::$names[$class] = \array_keys($publicConstants);
         }
 
         return self::$constants[$class];
     }
 
+    /**
+     * Assert that the given enumeration class doesn't define ambiguous enumerator values
+     * @param array $constants
+     * @return bool
+     */
+    private static function noAmbiguousValues(array $constants)
+    {
+        foreach ($constants as $value) {
+            $names = \array_keys($constants, $value, true);
+            if (\count($names) > 1) {
+                return false;
+            }
+        }
+
+        return true;
+    }
+
     /**
      * Get an enumerator instance by the given name.
      *

tests/MabeEnumTest/EnumTest.php

@@ -2,6 +2,7 @@
 
 namespace MabeEnumTest;
 
+use AssertionError;
 use InvalidArgumentException;
 use LogicException;
 use MabeEnum\Enum;
@@ -41,6 +42,11 @@ public function setUp()
         $instancesProp->setValue(null, []);
     }
 
+    public function tearDown()
+    {
+        assert_options(ASSERT_ACTIVE, 1);
+    }
+
     public function testGetNameReturnsConstantNameOfCurrentValue()
     {
         $enum = EnumBasic::get(EnumBasic::ONE);
@@ -274,15 +280,31 @@ public function testInstantiateUsingMagicMethod()
         $this->assertSame(EnumInheritance::ONE, $enum->getValue());
     }
 
-    public function testAmbiguousConstantsThrowsLogicException()
+    public function testEnabledAssertAmbiguousEnumeratorValues()
     {
-        $this->expectException(LogicException::class);
+        $this->expectException(AssertionError::class);
         EnumAmbiguous::get('unknown');
     }
 
-    public function testExtendedAmbiguousCanstantsThrowsLogicException()
+    public function testDisabledAssertAmbiguousEnumeratorValues()
     {
-        $this->expectException(LogicException::class);
+        assert_options(ASSERT_ACTIVE, 0);
+        $this->expectException(InvalidArgumentException::class);
+
+        EnumAmbiguous::get('unknown');
+    }
+
+    public function testExtendedEnabledAssertAmbiguousEnumeratorValues()
+    {
+        $this->expectException(AssertionError::class);
+        EnumExtendedAmbiguous::get('unknown');
+    }
+
+    public function testExtendedDisabledAssertAmbiguousEnumeratorValues()
+    {
+        assert_options(ASSERT_ACTIVE, 0);
+        $this->expectException(InvalidArgumentException::class);
+
         EnumExtendedAmbiguous::get('unknown');
     }
 
@@ -361,10 +383,6 @@ public function testConstVisibilityExtended()
 
     public function testIsSerializableIssue()
     {
-        if (PHP_VERSION_ID < 50400) {
-            $this->markTestSkipped('This test is for PHP-5.4 and upper only');
-        }
-
         $enum1 = SerializableEnum::INT();
         $enum2 = unserialize(serialize($enum1));
 

tests/bootstrap.php

@@ -1,6 +1,28 @@
 <?php
 
-ini_set('error_reporting', E_ALL);
+// report all errors
+error_reporting(E_ALL);
+
+// make sure zend.assertions are available (not disabled on compile time)
+$zendassertions = ini_get('zend.assertions');
+if (\PHP_VERSION_ID >= 70000 && $zendassertions == -1) {
+    echo 'Please enable zend.assertions in php.ini (zend.assertions = 1)' . PHP_EOL
+        . "Current ini setting: zend.assertions = {$zendassertions}]" . PHP_EOL;
+    exit(1);
+}
+
+// activate assertions
+assert_options(ASSERT_ACTIVE, 1);
+assert_options(ASSERT_WARNING, 0);
+assert_options(ASSERT_BAIL, 0);
+assert_options(ASSERT_QUIET_EVAL, 0);
+if (!class_exists('AssertionError')) {
+    // AssertionError has been added in PHP-7.0
+    class AssertionError extends Exception {};
+}
+assert_options(ASSERT_CALLBACK, function($file, $line, $code) {
+    throw new AssertionError("assert(): Assertion '{$code}' failed in {$file} on line {$line}");
+});
 
 // installed itself
 if (file_exists(__DIR__ . '/../vendor/autoload.php')) {
@@ -17,8 +39,8 @@
 }
 
 // autload test files
-spl_autoload_register(function ($className) {
-    $file = __DIR__ . '/' . str_replace('\\', '/', $className) . '.php';
+spl_autoload_register(function ($class) {
+    $file = __DIR__ . '/' . str_replace('\\', '/', $class) . '.php';
     if (file_exists($file)) {
         require $file;
     }