2 Minor Phpstan-related Fixes (#3030)

For one of the Phpstan upgrades, some message text had changed so drastically that the only practical solution at the time was to move the messages from phpstan-baseline.neon to phpstan.neon.dist. This was not ideal, but it allowed us time to move on and study the errors, which I have now done. At one point, Parser is expecting a variable to be an array, and that was not clear from the code. If not an array, the code will error out (which was Phpstan's concern); I have changed it to throw an exception instead. This satisfies Phpstan, and I can get the message out of neon.dist (without needing to restore it to baseline). Unsurprisingly, the exception was never thrown in the existing test suite, although I added a couple of tests to exercise that code.

In Helper/Dimension, Phpstan flagged a statement inappropriately. I suppressed the message using an annotation and filed a bug report phpstan/phpstan#7563. A fix for the problem was merged yesterday, which is good, but it puts us in a tenuous position. The annotation is needed now, but, when the fix is inevitably pushed to the version we use, the no-longer-needed annotation will trigger a different message. Recode so that neither the current nor the future versions will issue a message, eliminating the annotation in the process.

Author: oleibman

phpstan.neon.dist

@@ -21,8 +21,3 @@ parameters:
         # Accept a bit anything for assert methods
         - '~^Parameter \#2 .* of static method PHPUnit\\Framework\\Assert\:\:assert\w+\(\) expects .*, .* given\.$~'
         - '~^Method PhpOffice\\PhpSpreadsheetTests\\.*\:\:test.*\(\) has parameter \$args with no type specified\.$~'
-
-        # Some issues in Xls/Parser between 1.6.3 and 1.7.7
-        -
-            message: "#^Offset '(left|right|value)' does not exist on (non-empty-array\\|string|array\\|null)\\.$#"
-            path: src/PhpSpreadsheet/Writer/Xls/Parser.php

src/PhpSpreadsheet/Helper/Dimension.php

@@ -55,10 +55,20 @@ class Dimension
      */
     protected $unit;
 
+    /**
+     * Phpstan bug has been fixed; this function allows us to
+     * pass Phpstan whether fixed or not.
+     *
+     * @param mixed $value
+     */
+    private static function stanBugFixed($value): array
+    {
+        return is_array($value) ? $value : [null, null];
+    }
+
     public function __construct(string $dimension)
     {
-        // @phpstan-ignore-next-line
-        [$size, $unit] = sscanf($dimension, '%[1234567890.]%s');
+        [$size, $unit] = self::stanBugFixed(sscanf($dimension, '%[1234567890.]%s'));
         $unit = strtolower(trim($unit ?? ''));
         $size = (float) $size;
 

src/PhpSpreadsheet/Writer/Xls/Parser.php

@@ -78,7 +78,7 @@ class Parser
     /**
      * The parse tree to be generated.
      *
-     * @var string
+     * @var array|string
      */
     public $parseTree;
 
@@ -1445,6 +1445,9 @@ public function toReversePolish($tree = [])
         if (empty($tree)) { // If it's the first call use parseTree
             $tree = $this->parseTree;
         }
+        if (!is_array($tree) || !isset($tree['left'], $tree['right'], $tree['value'])) {
+            throw new WriterException('Unexpected non-array');
+        }
 
         if (is_array($tree['left'])) {
             $converted_tree = $this->toReversePolish($tree['left']);
@@ -1475,7 +1478,7 @@ public function toReversePolish($tree = [])
                 $left_tree = '';
             }
 
-            // add it's left subtree and return.
+            // add its left subtree and return.
             return $left_tree . $this->convertFunction($tree['value'], $tree['right']);
         }
         $converted_tree = $this->convert($tree['value']);

tests/PhpSpreadsheetTests/Writer/Xls/ParserTest.php

@@ -0,0 +1,56 @@
+<?php
+
+namespace PhpOffice\PhpSpreadsheetTests\Writer\Xls;
+
+use PhpOffice\PhpSpreadsheet\Spreadsheet;
+use PhpOffice\PhpSpreadsheet\Writer\Exception as WriterException;
+use PhpOffice\PhpSpreadsheet\Writer\Xls\Parser;
+use PHPUnit\Framework\TestCase;
+
+class ParserTest extends TestCase
+{
+    /** @var ?Spreadsheet */
+    private $spreadsheet;
+
+    protected function tearDown(): void
+    {
+        if ($this->spreadsheet !== null) {
+            $this->spreadsheet->disconnectWorksheets();
+            $this->spreadsheet = null;
+        }
+    }
+
+    public function testNonArray(): void
+    {
+        $this->expectException(WriterException::class);
+        $this->expectExceptionMessage('Unexpected non-array');
+        $this->spreadsheet = new Spreadsheet();
+        $parser = new Parser($this->spreadsheet);
+        $parser->toReversePolish();
+    }
+
+    public function testMissingIndex(): void
+    {
+        $this->expectException(WriterException::class);
+        $this->expectExceptionMessage('Unexpected non-array');
+        $this->spreadsheet = new Spreadsheet();
+        $parser = new Parser($this->spreadsheet);
+        $parser->toReversePolish(['left' => 0]);
+    }
+
+    public function testParseError(): void
+    {
+        $this->expectException(WriterException::class);
+        $this->expectExceptionMessage('Unknown token +');
+        $this->spreadsheet = new Spreadsheet();
+        $parser = new Parser($this->spreadsheet);
+        $parser->toReversePolish(['left' => 1, 'right' => 2, 'value' => '+']);
+    }
+
+    public function testGoodParse(): void
+    {
+        $this->spreadsheet = new Spreadsheet();
+        $parser = new Parser($this->spreadsheet);
+        self::assertSame('1e01001e02001e0300', bin2hex($parser->toReversePolish(['left' => 1, 'right' => 2, 'value' => 3])));
+    }
+}