Xls Writer - Correct Timestamp Bug, Improve Coverage (#1493)

* Xls Writer - Correct Timestamp Bug, Improve Coverage

I believe that Xls Writer is 100% covered now.

The Xls Writer sets its timestamp incorrectly. The problem is actually
in Shared/Ole::localDateToOLE, which converts its timestamp using
gmmktime; mktime is correct. If I save a file at 3:00 p.m. in San Francisco,
this bug means the time is actually recorded as 3:00 p.m. UTC.
A consequence of this is that if you use Phpspreadsheet to read the
file and save it as a new Xls, the creation timestamp goes further
and further back in time with each generation (or further forward
if east of Greenwich). One of the tests added confirms that
the creation timestamp is consistent with the start and end times
of the test.

The major change in coverage is adding tests to save GIF and BMP
images, which aren't supported in Xls, but are converted to PNG
in the PhpSpreadsheet code.

src/PhpSpreadsheet/Shared/OLE.php

@@ -505,7 +505,7 @@ class OLE
         // days from 1-1-1601 until the beggining of UNIX era
         $days = 134774;
         // calculate seconds
-        $big_date = $days * 24 * 3600 + gmmktime(date('H', $date), date('i', $date), date('s', $date), date('m', $date), date('d', $date), date('Y', $date));
+        $big_date = $days * 24 * 3600 + mktime((int) date('H', $date), (int) date('i', $date), (int) date('s', $date), (int) date('m', $date), (int) date('d', $date), (int) date('Y', $date));
         // multiply just to make MS happy
         $big_date *= 10000000;
 
@@ -558,10 +558,9 @@ class OLE
         // translate to seconds since 1970:
         $unixTimestamp = floor(65536.0 * 65536.0 * $timestampHigh + $timestampLow - $days * 24 * 3600 + 0.5);
 
-        if ((int) $unixTimestamp == $unixTimestamp) {
+        $iTimestamp = (int) $unixTimestamp;
-            return (int) $unixTimestamp;
-        }
 
-        return $unixTimestamp >= 0.0 ? PHP_INT_MAX : PHP_INT_MIN;
+        // Overflow conditions can't happen on 64-bit system
+        return ($iTimestamp == $unixTimestamp) ? $iTimestamp : ($unixTimestamp >= 0.0 ? PHP_INT_MAX : PHP_INT_MIN);
     }
 }

src/PhpSpreadsheet/Writer/Xls.php

@@ -23,7 +23,6 @@ use PhpOffice\PhpSpreadsheet\Spreadsheet;
 use PhpOffice\PhpSpreadsheet\Worksheet\BaseDrawing;
 use PhpOffice\PhpSpreadsheet\Worksheet\Drawing;
 use PhpOffice\PhpSpreadsheet\Worksheet\MemoryDrawing;
-use RuntimeException;
 
 class Xls extends BaseWriter
 {
@@ -389,74 +388,39 @@ class Xls extends BaseWriter
         }
     }
 
-    /**
+    private function processMemoryDrawing(BstoreContainer &$bstoreContainer, BaseDrawing $drawing, string $renderingFunctionx): void
-     * Build the Escher object corresponding to the MSODRAWINGGROUP record.
-     */
-    private function buildWorkbookEscher(): void
     {
-        $escher = null;
+        switch ($renderingFunctionx) {
+            case MemoryDrawing::RENDERING_JPEG:
+                $blipType = BSE::BLIPTYPE_JPEG;
+                $renderingFunction = 'imagejpeg';
 
-        // any drawings in this workbook?
+                break;
-        $found = false;
+            default:
-        foreach ($this->spreadsheet->getAllSheets() as $sheet) {
+                $blipType = BSE::BLIPTYPE_PNG;
-            if (count($sheet->getDrawingCollection()) > 0) {
+                $renderingFunction = 'imagepng';
-                $found = true;
 
                 break;
         }
+
+        ob_start();
+        call_user_func($renderingFunction, $drawing->getImageResource());
+        $blipData = ob_get_contents();
+        ob_end_clean();
+
+        $blip = new Blip();
+        $blip->setData($blipData);
+
+        $BSE = new BSE();
+        $BSE->setBlipType($blipType);
+        $BSE->setBlip($blip);
+
+        $bstoreContainer->addBSE($BSE);
     }
 
-        // nothing to do if there are no drawings
+    private function processDrawing(BstoreContainer &$bstoreContainer, BaseDrawing $drawing): void
-        if (!$found) {
+    {
-            return;
+        $blipData = '';
-        }
-
-        // if we reach here, then there are drawings in the workbook
-        $escher = new Escher();
-
-        // dggContainer
-        $dggContainer = new DggContainer();
-        $escher->setDggContainer($dggContainer);
-
-        // set IDCLs (identifier clusters)
-        $dggContainer->setIDCLs($this->IDCLs);
-
-        // this loop is for determining maximum shape identifier of all drawing
-        $spIdMax = 0;
-        $totalCountShapes = 0;
-        $countDrawings = 0;
-
-        foreach ($this->spreadsheet->getAllsheets() as $sheet) {
-            $sheetCountShapes = 0; // count number of shapes (minus group shape), in sheet
-
-            if (count($sheet->getDrawingCollection()) > 0) {
-                ++$countDrawings;
-
-                foreach ($sheet->getDrawingCollection() as $drawing) {
-                    ++$sheetCountShapes;
-                    ++$totalCountShapes;
-
-                    $spId = $sheetCountShapes | ($this->spreadsheet->getIndex($sheet) + 1) << 10;
-                    $spIdMax = max($spId, $spIdMax);
-                }
-            }
-        }
-
-        $dggContainer->setSpIdMax($spIdMax + 1);
-        $dggContainer->setCDgSaved($countDrawings);
-        $dggContainer->setCSpSaved($totalCountShapes + $countDrawings); // total number of shapes incl. one group shapes per drawing
-
-        // bstoreContainer
-        $bstoreContainer = new BstoreContainer();
-        $dggContainer->setBstoreContainer($bstoreContainer);
-
-        // the BSE's (all the images)
-        foreach ($this->spreadsheet->getAllsheets() as $sheet) {
-            foreach ($sheet->getDrawingCollection() as $drawing) {
-                if (!extension_loaded('gd')) {
-                    throw new RuntimeException('Saving images in xls requires gd extension');
-                }
-                if ($drawing instanceof Drawing) {
         $filename = $drawing->getPath();
 
         [$imagesx, $imagesy, $imageFormat] = getimagesize($filename);
@@ -488,10 +452,8 @@ class Xls extends BaseWriter
                 ob_end_clean();
 
                 break;
-                        default:
-                            continue 2;
         }
-
+        if ($blipData) {
             $blip = new Blip();
             $blip->setData($blipData);
 
@@ -500,36 +462,85 @@ class Xls extends BaseWriter
             $BSE->setBlip($blip);
 
             $bstoreContainer->addBSE($BSE);
+        }
+    }
+
+    private function processBaseDrawing(BstoreContainer &$bstoreContainer, BaseDrawing $drawing): void
+    {
+        if ($drawing instanceof Drawing) {
+            $this->processDrawing($bstoreContainer, $drawing);
         } elseif ($drawing instanceof MemoryDrawing) {
-                    switch ($drawing->getRenderingFunction()) {
+            $this->processMemoryDrawing($bstoreContainer, $drawing, $drawing->getRenderingFunction());
-                        case MemoryDrawing::RENDERING_JPEG:
+        }
-                            $blipType = BSE::BLIPTYPE_JPEG;
+    }
-                            $renderingFunction = 'imagejpeg';
 
-                            break;
+    private function checkForDrawings(): bool
-                        case MemoryDrawing::RENDERING_GIF:
+    {
-                        case MemoryDrawing::RENDERING_PNG:
+        // any drawings in this workbook?
-                        case MemoryDrawing::RENDERING_DEFAULT:
+        $found = false;
-                            $blipType = BSE::BLIPTYPE_PNG;
+        foreach ($this->spreadsheet->getAllSheets() as $sheet) {
-                            $renderingFunction = 'imagepng';
+            if (count($sheet->getDrawingCollection()) > 0) {
+                $found = true;
 
                 break;
             }
-
-                    ob_start();
-                    call_user_func($renderingFunction, $drawing->getImageResource());
-                    $blipData = ob_get_contents();
-                    ob_end_clean();
-
-                    $blip = new Blip();
-                    $blip->setData($blipData);
-
-                    $BSE = new BSE();
-                    $BSE->setBlipType($blipType);
-                    $BSE->setBlip($blip);
-
-                    $bstoreContainer->addBSE($BSE);
         }
+
+        return $found;
+    }
+
+    /**
+     * Build the Escher object corresponding to the MSODRAWINGGROUP record.
+     */
+    private function buildWorkbookEscher(): void
+    {
+        // nothing to do if there are no drawings
+        if (!$this->checkForDrawings()) {
+            return;
+        }
+
+        // if we reach here, then there are drawings in the workbook
+        $escher = new Escher();
+
+        // dggContainer
+        $dggContainer = new DggContainer();
+        $escher->setDggContainer($dggContainer);
+
+        // set IDCLs (identifier clusters)
+        $dggContainer->setIDCLs($this->IDCLs);
+
+        // this loop is for determining maximum shape identifier of all drawing
+        $spIdMax = 0;
+        $totalCountShapes = 0;
+        $countDrawings = 0;
+
+        foreach ($this->spreadsheet->getAllsheets() as $sheet) {
+            $sheetCountShapes = 0; // count number of shapes (minus group shape), in sheet
+
+            $addCount = 0;
+            foreach ($sheet->getDrawingCollection() as $drawing) {
+                $addCount = 1;
+                ++$sheetCountShapes;
+                ++$totalCountShapes;
+
+                $spId = $sheetCountShapes | ($this->spreadsheet->getIndex($sheet) + 1) << 10;
+                $spIdMax = max($spId, $spIdMax);
+            }
+            $countDrawings += $addCount;
+        }
+
+        $dggContainer->setSpIdMax($spIdMax + 1);
+        $dggContainer->setCDgSaved($countDrawings);
+        $dggContainer->setCSpSaved($totalCountShapes + $countDrawings); // total number of shapes incl. one group shapes per drawing
+
+        // bstoreContainer
+        $bstoreContainer = new BstoreContainer();
+        $dggContainer->setBstoreContainer($bstoreContainer);
+
+        // the BSE's (all the images)
+        foreach ($this->spreadsheet->getAllsheets() as $sheet) {
+            foreach ($sheet->getDrawingCollection() as $drawing) {
+                $this->processBaseDrawing($bstoreContainer, $drawing);
             }
         }
 
@@ -578,8 +589,8 @@ class Xls extends BaseWriter
         ++$dataSection_NumProps;
 
         // GKPIDDSI_CATEGORY : Category
-        if ($this->spreadsheet->getProperties()->getCategory()) {
         $dataProp = $this->spreadsheet->getProperties()->getCategory();
+        if ($dataProp) {
             $dataSection[] = [
                 'summary' => ['pack' => 'V', 'data' => 0x02],
                 'offset' => ['pack' => 'V'],
@@ -707,11 +718,7 @@ class Xls extends BaseWriter
 
                 $dataSection_Content_Offset += 4 + 4;
             } elseif ($dataProp['type']['data'] == 0x0B) { // Boolean
-                if ($dataProp['data']['data'] == false) {
+                $dataSection_Content .= pack('V', (int) $dataProp['data']['data']);
-                    $dataSection_Content .= pack('V', 0x0000);
-                } else {
-                    $dataSection_Content .= pack('V', 0x0001);
-                }
                 $dataSection_Content_Offset += 4 + 4;
             } elseif ($dataProp['type']['data'] == 0x1E) { // null-terminated string prepended by dword string length
                 // Null-terminated string
@@ -725,12 +732,12 @@ class Xls extends BaseWriter
                 $dataSection_Content .= $dataProp['data']['data'];
 
                 $dataSection_Content_Offset += 4 + 4 + strlen($dataProp['data']['data']);
-            } elseif ($dataProp['type']['data'] == 0x40) { // Filetime (64-bit value representing the number of 100-nanosecond intervals since January 1, 1601)
+            // Condition below can never be true
-                $dataSection_Content .= $dataProp['data']['data'];
+            //} elseif ($dataProp['type']['data'] == 0x40) { // Filetime (64-bit value representing the number of 100-nanosecond intervals since January 1, 1601)
+            //    $dataSection_Content .= $dataProp['data']['data'];
 
-                $dataSection_Content_Offset += 4 + 8;
+            //    $dataSection_Content_Offset += 4 + 8;
             } else {
-                // Data Type Not Used at the moment
                 $dataSection_Content .= $dataProp['data']['data'];
 
                 $dataSection_Content_Offset += 4 + $dataProp['data']['length'];
@@ -752,6 +759,32 @@ class Xls extends BaseWriter
         return $data;
     }
 
+    private function writeSummaryPropOle(int $dataProp, int &$dataSection_NumProps, array &$dataSection, int $sumdata, int $typdata): void
+    {
+        if ($dataProp) {
+            $dataSection[] = [
+                'summary' => ['pack' => 'V', 'data' => $sumdata],
+                'offset' => ['pack' => 'V'],
+                'type' => ['pack' => 'V', 'data' => $typdata], // null-terminated string prepended by dword string length
+                'data' => ['data' => OLE::localDateToOLE($dataProp)],
+            ];
+            ++$dataSection_NumProps;
+        }
+    }
+
+    private function writeSummaryProp(string $dataProp, int &$dataSection_NumProps, array &$dataSection, int $sumdata, int $typdata): void
+    {
+        if ($dataProp) {
+            $dataSection[] = [
+                'summary' => ['pack' => 'V', 'data' => $sumdata],
+                'offset' => ['pack' => 'V'],
+                'type' => ['pack' => 'V', 'data' => $typdata], // null-terminated string prepended by dword string length
+                'data' => ['data' => $dataProp, 'length' => strlen($dataProp)],
+            ];
+            ++$dataSection_NumProps;
+        }
+    }
+
     /**
      * Build the OLE Part for Summary Information.
      *
@@ -792,94 +825,16 @@ class Xls extends BaseWriter
         ];
         ++$dataSection_NumProps;
 
-        //    Title
+        $props = $this->spreadsheet->getProperties();
-        if ($this->spreadsheet->getProperties()->getTitle()) {
+        $this->writeSummaryProp($props->getTitle(), $dataSection_NumProps, $dataSection, 0x02, 0x1e);
-            $dataProp = $this->spreadsheet->getProperties()->getTitle();
+        $this->writeSummaryProp($props->getSubject(), $dataSection_NumProps, $dataSection, 0x03, 0x1e);
-            $dataSection[] = [
+        $this->writeSummaryProp($props->getCreator(), $dataSection_NumProps, $dataSection, 0x04, 0x1e);
-                'summary' => ['pack' => 'V', 'data' => 0x02],
+        $this->writeSummaryProp($props->getKeywords(), $dataSection_NumProps, $dataSection, 0x05, 0x1e);
-                'offset' => ['pack' => 'V'],
+        $this->writeSummaryProp($props->getDescription(), $dataSection_NumProps, $dataSection, 0x06, 0x1e);
-                'type' => ['pack' => 'V', 'data' => 0x1E], // null-terminated string prepended by dword string length
+        $this->writeSummaryProp($props->getLastModifiedBy(), $dataSection_NumProps, $dataSection, 0x08, 0x1e);
-                'data' => ['data' => $dataProp, 'length' => strlen($dataProp)],
+        $this->writeSummaryPropOle($props->getCreated(), $dataSection_NumProps, $dataSection, 0x0c, 0x40);
-            ];
+        $this->writeSummaryPropOle($props->getModified(), $dataSection_NumProps, $dataSection, 0x0d, 0x40);
-            ++$dataSection_NumProps;
+
-        }
-        //    Subject
-        if ($this->spreadsheet->getProperties()->getSubject()) {
-            $dataProp = $this->spreadsheet->getProperties()->getSubject();
-            $dataSection[] = [
-                'summary' => ['pack' => 'V', 'data' => 0x03],
-                'offset' => ['pack' => 'V'],
-                'type' => ['pack' => 'V', 'data' => 0x1E], // null-terminated string prepended by dword string length
-                'data' => ['data' => $dataProp, 'length' => strlen($dataProp)],
-            ];
-            ++$dataSection_NumProps;
-        }
-        //    Author (Creator)
-        if ($this->spreadsheet->getProperties()->getCreator()) {
-            $dataProp = $this->spreadsheet->getProperties()->getCreator();
-            $dataSection[] = [
-                'summary' => ['pack' => 'V', 'data' => 0x04],
-                'offset' => ['pack' => 'V'],
-                'type' => ['pack' => 'V', 'data' => 0x1E], // null-terminated string prepended by dword string length
-                'data' => ['data' => $dataProp, 'length' => strlen($dataProp)],
-            ];
-            ++$dataSection_NumProps;
-        }
-        //    Keywords
-        if ($this->spreadsheet->getProperties()->getKeywords()) {
-            $dataProp = $this->spreadsheet->getProperties()->getKeywords();
-            $dataSection[] = [
-                'summary' => ['pack' => 'V', 'data' => 0x05],
-                'offset' => ['pack' => 'V'],
-                'type' => ['pack' => 'V', 'data' => 0x1E], // null-terminated string prepended by dword string length
-                'data' => ['data' => $dataProp, 'length' => strlen($dataProp)],
-            ];
-            ++$dataSection_NumProps;
-        }
-        //    Comments (Description)
-        if ($this->spreadsheet->getProperties()->getDescription()) {
-            $dataProp = $this->spreadsheet->getProperties()->getDescription();
-            $dataSection[] = [
-                'summary' => ['pack' => 'V', 'data' => 0x06],
-                'offset' => ['pack' => 'V'],
-                'type' => ['pack' => 'V', 'data' => 0x1E], // null-terminated string prepended by dword string length
-                'data' => ['data' => $dataProp, 'length' => strlen($dataProp)],
-            ];
-            ++$dataSection_NumProps;
-        }
-        //    Last Saved By (LastModifiedBy)
-        if ($this->spreadsheet->getProperties()->getLastModifiedBy()) {
-            $dataProp = $this->spreadsheet->getProperties()->getLastModifiedBy();
-            $dataSection[] = [
-                'summary' => ['pack' => 'V', 'data' => 0x08],
-                'offset' => ['pack' => 'V'],
-                'type' => ['pack' => 'V', 'data' => 0x1E], // null-terminated string prepended by dword string length
-                'data' => ['data' => $dataProp, 'length' => strlen($dataProp)],
-            ];
-            ++$dataSection_NumProps;
-        }
-        //    Created Date/Time
-        if ($this->spreadsheet->getProperties()->getCreated()) {
-            $dataProp = $this->spreadsheet->getProperties()->getCreated();
-            $dataSection[] = [
-                'summary' => ['pack' => 'V', 'data' => 0x0C],
-                'offset' => ['pack' => 'V'],
-                'type' => ['pack' => 'V', 'data' => 0x40], // Filetime (64-bit value representing the number of 100-nanosecond intervals since January 1, 1601)
-                'data' => ['data' => OLE::localDateToOLE($dataProp)],
-            ];
-            ++$dataSection_NumProps;
-        }
-        //    Modified Date/Time
-        if ($this->spreadsheet->getProperties()->getModified()) {
-            $dataProp = $this->spreadsheet->getProperties()->getModified();
-            $dataSection[] = [
-                'summary' => ['pack' => 'V', 'data' => 0x0D],
-                'offset' => ['pack' => 'V'],
-                'type' => ['pack' => 'V', 'data' => 0x40], // Filetime (64-bit value representing the number of 100-nanosecond intervals since January 1, 1601)
-                'data' => ['data' => OLE::localDateToOLE($dataProp)],
-            ];
-            ++$dataSection_NumProps;
-        }
         //    Security
         $dataSection[] = [
             'summary' => ['pack' => 'V', 'data' => 0x13],

tests/PhpSpreadsheetTests/Writer/Xls/XlsGifBmpTest.php

@@ -0,0 +1,84 @@
+<?php
+
+namespace PhpOffice\PhpSpreadsheetTests\Writer\Xls;
+
+use PhpOffice\PhpSpreadsheet\Spreadsheet;
+use PhpOffice\PhpSpreadsheet\Worksheet\Drawing;
+use PhpOffice\PhpSpreadsheet\Worksheet\MemoryDrawing;
+use PhpOffice\PhpSpreadsheetTests\Functional\AbstractFunctional;
+
+class XlsGifBmpTest extends AbstractFunctional
+{
+    private $filename = '';
+
+    protected function tearDown(): void
+    {
+        if ($this->filename) {
+            unlink($this->filename);
+        }
+        $this->filename = '';
+    }
+
+    public function testBmp(): void
+    {
+        $pgmstart = time();
+        $spreadsheet = new Spreadsheet();
+        $filstart = $spreadsheet->getProperties()->getModified();
+        self::assertLessThanOrEqual($filstart, $pgmstart);
+
+        // Add a drawing to the worksheet
+        $drawing = new Drawing();
+        $drawing->setName('Letters B, M, and P');
+        $drawing->setDescription('Handwritten B, M, and P');
+        $drawing->setPath(__DIR__ . '/../../../../samples/images/bmp.bmp');
+        $drawing->setHeight(36);
+        $drawing->setWorksheet($spreadsheet->getActiveSheet());
+        $drawing->setCoordinates('A1');
+
+        $reloadedSpreadsheet = $this->writeAndReload($spreadsheet, 'Xls');
+        $creationDatestamp = $reloadedSpreadsheet->getProperties()->getCreated();
+        $filstart = $creationDatestamp;
+        $pSheet = $reloadedSpreadsheet->getActiveSheet();
+        $drawings = $pSheet->getDrawingCollection();
+        self::assertCount(1, $drawings);
+        foreach ($pSheet->getDrawingCollection() as $drawing) {
+            // See if Scrutinizer approves this
+            $mimeType = ($drawing instanceof MemoryDrawing) ? $drawing->getMimeType() : 'notmemorydrawing';
+            self::assertEquals('image/png', $mimeType);
+        }
+        $pgmend = time();
+
+        self::assertLessThanOrEqual($pgmend, $pgmstart);
+        self::assertLessThanOrEqual($pgmend, $filstart);
+        self::assertLessThanOrEqual($filstart, $pgmstart);
+    }
+
+    public function testGif(): void
+    {
+        $spreadsheet = new Spreadsheet();
+
+        // Add a drawing to the worksheet
+        $drawing = new Drawing();
+        $drawing->setName('Letters G, I, and G');
+        $drawing->setDescription('Handwritten G, I, and F');
+        $drawing->setPath(__DIR__ . '/../../../../samples/images/gif.gif');
+        $drawing->setHeight(36);
+        $drawing->setWorksheet($spreadsheet->getActiveSheet());
+        $drawing->setCoordinates('A1');
+
+        $reloadedSpreadsheet = $this->writeAndReload($spreadsheet, 'Xls');
+        $pSheet = $reloadedSpreadsheet->getActiveSheet();
+        $drawings = $pSheet->getDrawingCollection();
+        self::assertCount(1, $drawings);
+        foreach ($pSheet->getDrawingCollection() as $drawing) {
+            $mimeType = ($drawing instanceof MemoryDrawing) ? $drawing->getMimeType() : 'notmemorydrawing';
+            self::assertEquals('image/png', $mimeType);
+        }
+    }
+
+    public function testInvalidTimestamp(): void
+    {
+        $this->expectException(\PhpOffice\PhpSpreadsheet\Reader\Exception::class);
+        \PhpOffice\PhpSpreadsheet\Shared\OLE::OLE2LocalDate(' ');
+    }
+}