Advanced Value Binder Improvements (#1862)

Advanced Value Binder
 - Improved format checking/setting for fractions;
 - Better percentage checking;
 - Some minor refactoring;
 - Improved unit testing

src/PhpSpreadsheet/Cell/AdvancedValueBinder.php

@@ -20,8 +20,10 @@ class AdvancedValueBinder extends DefaultValueBinder implements IValueBinder
      */
     public function bindValue(Cell $cell, $value = null)
     {
-        // sanitize UTF-8 strings
+        if ($value === null) {
-        if (is_string($value)) {
+            return parent::bindValue($cell, $value);
+        } elseif (is_string($value)) {
+            // sanitize UTF-8 strings
             $value = StringHelper::sanitizeUTF8($value);
         }
 
@@ -41,50 +43,16 @@ class AdvancedValueBinder extends DefaultValueBinder implements IValueBinder
                 return true;
             }
 
-            // Check for number in scientific format
+            // Check for fractions
-            if (preg_match('/^' . Calculation::CALCULATION_REGEXP_NUMBER . '$/', $value)) {
-                $cell->setValueExplicit((float) $value, DataType::TYPE_NUMERIC);
-
-                return true;
-            }
-
-            // Check for fraction
             if (preg_match('/^([+-]?)\s*(\d+)\s?\/\s*(\d+)$/', $value, $matches)) {
-                // Convert value to number
+                return $this->setProperFraction($matches, $cell);
-                $value = $matches[2] / $matches[3];
-                if ($matches[1] == '-') {
-                    $value = 0 - $value;
-                }
-                $cell->setValueExplicit((float) $value, DataType::TYPE_NUMERIC);
-                // Set style
-                $cell->getWorksheet()->getStyle($cell->getCoordinate())
-                    ->getNumberFormat()->setFormatCode('??/??');
-
-                return true;
             } elseif (preg_match('/^([+-]?)(\d*) +(\d*)\s?\/\s*(\d*)$/', $value, $matches)) {
-                // Convert value to number
+                return $this->setImproperFraction($matches, $cell);
-                $value = $matches[2] + ($matches[3] / $matches[4]);
-                if ($matches[1] == '-') {
-                    $value = 0 - $value;
-                }
-                $cell->setValueExplicit((float) $value, DataType::TYPE_NUMERIC);
-                // Set style
-                $cell->getWorksheet()->getStyle($cell->getCoordinate())
-                    ->getNumberFormat()->setFormatCode('# ??/??');
-
-                return true;
             }
 
             // Check for percentage
             if (preg_match('/^\-?\d*\.?\d*\s?\%$/', $value)) {
-                // Convert value to number
+                return $this->setPercentage($value, $cell);
-                $value = (float) str_replace('%', '', $value) / 100;
-                $cell->setValueExplicit($value, DataType::TYPE_NUMERIC);
-                // Set style
-                $cell->getWorksheet()->getStyle($cell->getCoordinate())
-                    ->getNumberFormat()->setFormatCode(NumberFormat::FORMAT_PERCENTAGE_00);
-
-                return true;
             }
 
             // Check for currency
@@ -158,7 +126,6 @@ class AdvancedValueBinder extends DefaultValueBinder implements IValueBinder
 
             // Check for newline character "\n"
             if (strpos($value, "\n") !== false) {
-                $value = StringHelper::sanitizeUTF8($value);
                 $cell->setValueExplicit($value, DataType::TYPE_STRING);
                 // Set style
                 $cell->getWorksheet()->getStyle($cell->getCoordinate())
@@ -171,4 +138,57 @@ class AdvancedValueBinder extends DefaultValueBinder implements IValueBinder
         // Not bound yet? Use parent...
         return parent::bindValue($cell, $value);
     }
+
+    protected function setImproperFraction(array $matches, Cell $cell): bool
+    {
+        // Convert value to number
+        $value = $matches[2] + ($matches[3] / $matches[4]);
+        if ($matches[1] === '-') {
+            $value = 0 - $value;
+        }
+        $cell->setValueExplicit((float) $value, DataType::TYPE_NUMERIC);
+
+        // Build the number format mask based on the size of the matched values
+        $dividend = str_repeat('?', strlen($matches[3]));
+        $divisor = str_repeat('?', strlen($matches[4]));
+        $fractionMask = "# {$dividend}/{$divisor}";
+        // Set style
+        $cell->getWorksheet()->getStyle($cell->getCoordinate())
+            ->getNumberFormat()->setFormatCode($fractionMask);
+
+        return true;
+    }
+
+    protected function setProperFraction(array $matches, Cell $cell): bool
+    {
+        // Convert value to number
+        $value = $matches[2] / $matches[3];
+        if ($matches[1] === '-') {
+            $value = 0 - $value;
+        }
+        $cell->setValueExplicit((float) $value, DataType::TYPE_NUMERIC);
+
+        // Build the number format mask based on the size of the matched values
+        $dividend = str_repeat('?', strlen($matches[2]));
+        $divisor = str_repeat('?', strlen($matches[3]));
+        $fractionMask = "{$dividend}/{$divisor}";
+        // Set style
+        $cell->getWorksheet()->getStyle($cell->getCoordinate())
+            ->getNumberFormat()->setFormatCode($fractionMask);
+
+        return true;
+    }
+
+    protected function setPercentage(string $value, Cell $cell): bool
+    {
+        // Convert value to number
+        $value = ((float) str_replace('%', '', $value)) / 100;
+        $cell->setValueExplicit($value, DataType::TYPE_NUMERIC);
+
+        // Set style
+        $cell->getWorksheet()->getStyle($cell->getCoordinate())
+            ->getNumberFormat()->setFormatCode(NumberFormat::FORMAT_PERCENTAGE_00);
+
+        return true;
+    }
 }

src/PhpSpreadsheet/Cell/DefaultValueBinder.php

@@ -40,39 +40,39 @@ class DefaultValueBinder implements IValueBinder
     /**
      * DataType for value.
      *
-     * @param mixed $pValue
+     * @param mixed $value
      *
      * @return string
      */
-    public static function dataTypeForValue($pValue)
+    public static function dataTypeForValue($value)
     {
         // Match the value against a few data types
-        if ($pValue === null) {
+        if ($value === null) {
             return DataType::TYPE_NULL;
-        } elseif (is_float($pValue) || is_int($pValue)) {
+        } elseif (is_float($value) || is_int($value)) {
             return DataType::TYPE_NUMERIC;
-        } elseif (is_bool($pValue)) {
+        } elseif (is_bool($value)) {
             return DataType::TYPE_BOOL;
-        } elseif ($pValue === '') {
+        } elseif ($value === '') {
             return DataType::TYPE_STRING;
-        } elseif ($pValue instanceof RichText) {
+        } elseif ($value instanceof RichText) {
             return DataType::TYPE_INLINE;
-        } elseif (is_string($pValue) && $pValue[0] === '=' && strlen($pValue) > 1) {
+        } elseif (is_string($value) && $value[0] === '=' && strlen($value) > 1) {
             return DataType::TYPE_FORMULA;
-        } elseif (preg_match('/^[\+\-]?(\d+\\.?\d*|\d*\\.?\d+)([Ee][\-\+]?[0-2]?\d{1,3})?$/', $pValue)) {
+        } elseif (preg_match('/^[\+\-]?(\d+\\.?\d*|\d*\\.?\d+)([Ee][\-\+]?[0-2]?\d{1,3})?$/', $value)) {
-            $tValue = ltrim($pValue, '+-');
+            $tValue = ltrim($value, '+-');
-            if (is_string($pValue) && $tValue[0] === '0' && strlen($tValue) > 1 && $tValue[1] !== '.') {
+            if (is_string($value) && $tValue[0] === '0' && strlen($tValue) > 1 && $tValue[1] !== '.') {
                 return DataType::TYPE_STRING;
-            } elseif ((strpos($pValue, '.') === false) && ($pValue > PHP_INT_MAX)) {
+            } elseif ((strpos($value, '.') === false) && ($value > PHP_INT_MAX)) {
                 return DataType::TYPE_STRING;
-            } elseif (!is_numeric($pValue)) {
+            } elseif (!is_numeric($value)) {
                 return DataType::TYPE_STRING;
             }
 
             return DataType::TYPE_NUMERIC;
-        } elseif (is_string($pValue)) {
+        } elseif (is_string($value)) {
             $errorCodes = DataType::getErrorCodes();
-            if (isset($errorCodes[$pValue])) {
+            if (isset($errorCodes[$value])) {
                 return DataType::TYPE_ERROR;
             }
         }

tests/PhpSpreadsheetTests/Cell/AdvancedValueBinderTest.php

@@ -33,25 +33,8 @@ class AdvancedValueBinderTest extends TestCase
         StringHelper::setThousandsSeparator($this->thousandsSeparator);
     }
 
-    public function provider()
-    {
-        $currencyUSD = NumberFormat::FORMAT_CURRENCY_USD_SIMPLE;
-        $currencyEURO = str_replace('$', '€', NumberFormat::FORMAT_CURRENCY_USD_SIMPLE);
-
-        return [
-            ['10%', 0.1, NumberFormat::FORMAT_PERCENTAGE_00, ',', '.', '$'],
-            ['$10.11', 10.11, $currencyUSD, ',', '.', '$'],
-            ['$1,010.12', 1010.12, $currencyUSD, ',', '.', '$'],
-            ['$20,20', 20.2, $currencyUSD, '.', ',', '$'],
-            ['$2.020,20', 2020.2, $currencyUSD, '.', ',', '$'],
-            ['€2.020,20', 2020.2, $currencyEURO, '.', ',', '€'],
-            ['€ 2.020,20', 2020.2, $currencyEURO, '.', ',', '€'],
-            ['€2,020.22', 2020.22, $currencyEURO, ',', '.', '€'],
-        ];
-    }
-
     /**
-     * @dataProvider provider
+     * @dataProvider currencyProvider
      *
      * @param mixed $value
      * @param mixed $valueBinded
@@ -96,4 +79,173 @@ class AdvancedValueBinderTest extends TestCase
         $binder->bindValue($cell, $value);
         self::assertEquals($valueBinded, $cell->getValue());
     }
+
+    public function currencyProvider()
+    {
+        $currencyUSD = NumberFormat::FORMAT_CURRENCY_USD_SIMPLE;
+        $currencyEURO = str_replace('$', '€', NumberFormat::FORMAT_CURRENCY_USD_SIMPLE);
+
+        return [
+            ['$10.11', 10.11, $currencyUSD, ',', '.', '$'],
+            ['$1,010.12', 1010.12, $currencyUSD, ',', '.', '$'],
+            ['$20,20', 20.2, $currencyUSD, '.', ',', '$'],
+            ['$2.020,20', 2020.2, $currencyUSD, '.', ',', '$'],
+            ['€2.020,20', 2020.2, $currencyEURO, '.', ',', '€'],
+            ['€ 2.020,20', 2020.2, $currencyEURO, '.', ',', '€'],
+            ['€2,020.22', 2020.22, $currencyEURO, ',', '.', '€'],
+        ];
+    }
+
+    /**
+     * @dataProvider fractionProvider
+     *
+     * @param mixed $value
+     * @param mixed $valueBinded
+     * @param mixed $format
+     */
+    public function testFractions($value, $valueBinded, $format): void
+    {
+        $sheet = $this->getMockBuilder(Worksheet::class)
+            ->setMethods(['getStyle', 'getNumberFormat', 'setFormatCode', 'getCellCollection'])
+            ->getMock();
+
+        $cellCollection = $this->getMockBuilder(Cells::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $cellCollection->expects(self::any())
+            ->method('getParent')
+            ->willReturn($sheet);
+
+        $sheet->expects(self::once())
+            ->method('getStyle')
+            ->willReturnSelf();
+        $sheet->expects(self::once())
+            ->method('getNumberFormat')
+            ->willReturnSelf();
+        $sheet->expects(self::once())
+            ->method('setFormatCode')
+            ->with($format)
+            ->willReturnSelf();
+        $sheet->expects(self::any())
+            ->method('getCellCollection')
+            ->willReturn($cellCollection);
+
+        $cell = new Cell(null, DataType::TYPE_STRING, $sheet);
+
+        $binder = new AdvancedValueBinder();
+        $binder->bindValue($cell, $value);
+        self::assertEquals($valueBinded, $cell->getValue());
+    }
+
+    public function fractionProvider()
+    {
+        return [
+            ['1/5', 0.2, '?/?'],
+            ['-1/5', -0.2, '?/?'],
+            ['12/5', 2.4, '??/?'],
+            ['2/100', 0.02, '?/???'],
+            ['15/12', 1.25, '??/??'],
+            ['20/100', 0.2, '??/???'],
+            ['1 3/5', 1.6, '# ?/?'],
+            ['-1 3/5', -1.6, '# ?/?'],
+            ['1 4/20', 1.2, '# ?/??'],
+            ['1 16/20', 1.8, '# ??/??'],
+            ['12 20/100', 12.2, '# ??/???'],
+        ];
+    }
+
+    /**
+     * @dataProvider percentageProvider
+     *
+     * @param mixed $value
+     * @param mixed $valueBinded
+     * @param mixed $format
+     */
+    public function testPercentages($value, $valueBinded, $format): void
+    {
+        $sheet = $this->getMockBuilder(Worksheet::class)
+            ->setMethods(['getStyle', 'getNumberFormat', 'setFormatCode', 'getCellCollection'])
+            ->getMock();
+        $cellCollection = $this->getMockBuilder(Cells::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $cellCollection->expects(self::any())
+            ->method('getParent')
+            ->willReturn($sheet);
+
+        $sheet->expects(self::once())
+            ->method('getStyle')
+            ->willReturnSelf();
+        $sheet->expects(self::once())
+            ->method('getNumberFormat')
+            ->willReturnSelf();
+        $sheet->expects(self::once())
+            ->method('setFormatCode')
+            ->with($format)
+            ->willReturnSelf();
+        $sheet->expects(self::any())
+            ->method('getCellCollection')
+            ->willReturn($cellCollection);
+
+        $cell = new Cell(null, DataType::TYPE_STRING, $sheet);
+
+        $binder = new AdvancedValueBinder();
+        $binder->bindValue($cell, $value);
+        self::assertEquals($valueBinded, $cell->getValue());
+    }
+
+    public function percentageProvider()
+    {
+        return [
+            ['10%', 0.1, NumberFormat::FORMAT_PERCENTAGE_00],
+            ['-12%', -0.12, NumberFormat::FORMAT_PERCENTAGE_00],
+            ['120%', 1.2, NumberFormat::FORMAT_PERCENTAGE_00],
+        ];
+    }
+
+    /**
+     * @dataProvider stringProvider
+     *
+     * @param mixed $value
+     * @param mixed $wrapped
+     */
+    public function testStringWrapping(string $value, bool $wrapped): void
+    {
+        $sheet = $this->getMockBuilder(Worksheet::class)
+            ->setMethods(['getStyle', 'getAlignment', 'setWrapText', 'getCellCollection'])
+            ->getMock();
+        $cellCollection = $this->getMockBuilder(Cells::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $cellCollection->expects(self::any())
+            ->method('getParent')
+            ->willReturn($sheet);
+
+        $sheet->expects($wrapped ? self::once() : self::never())
+            ->method('getStyle')
+            ->willReturnSelf();
+        $sheet->expects($wrapped ? self::once() : self::never())
+            ->method('getAlignment')
+            ->willReturnSelf();
+        $sheet->expects($wrapped ? self::once() : self::never())
+            ->method('setWrapText')
+            ->with($wrapped)
+            ->willReturnSelf();
+        $sheet->expects(self::any())
+            ->method('getCellCollection')
+            ->willReturn($cellCollection);
+
+        $cell = new Cell(null, DataType::TYPE_STRING, $sheet);
+
+        $binder = new AdvancedValueBinder();
+        $binder->bindValue($cell, $value);
+    }
+
+    public function stringProvider()
+    {
+        return [
+            ['Hello World', false],
+            ["Hello\nWorld", true],
+        ];
+    }
 }

tests/PhpSpreadsheetTests/Cell/ValueBinderWithOverriddenDataTypeForValue.php

@@ -8,10 +8,10 @@ class ValueBinderWithOverriddenDataTypeForValue extends DefaultValueBinder
 {
     public static $called = false;
 
-    public static function dataTypeForValue($pValue)
+    public static function dataTypeForValue($value)
     {
         self::$called = true;
 
-        return parent::dataTypeForValue($pValue);
+        return parent::dataTypeForValue($value);
     }
 }