Merge pull request #147 from FriendsOfCake/fix-5x-pr146-cs-stan

Fix CsvView state reset and 5.x cs-stan setup

Author: dereuromark

README.md

@@ -180,6 +180,10 @@ to set how null values should be displayed in the CSV.
 
 `null` defaults to `''`.
 
+Extract paths now consistently resolve through `Hash::get()`, so missing keys
+become `null`. If an extract path resolves to an array or object that cannot be
+stringified, use a callable extractor to flatten it before rendering.
+
 #### Automatic view class switching
 
 You can use the controller's content negotiation feature to automatically have
@@ -328,4 +332,3 @@ $view->set(compact('data'));
 // And Save the file
 file_put_contents('/full/path/to/file.csv', $view->render());
 ```
-

phpcs.xml

@@ -1,6 +1,5 @@
 <?xml version="1.0"?>
 <ruleset name="CsvView">
-    <config name="installed_paths" value="../../cakephp/cakephp-codesniffer" />
     <arg value="s"/>
 
     <file>src/</file>

phpstan.neon

@@ -4,5 +4,3 @@ parameters:
         - src/
     bootstrapFiles:
         - tests/bootstrap.php
-    ignoreErrors:
-        - identifier: missingType.generics

src/View/CsvView.php

@@ -7,6 +7,7 @@
 use Cake\Datasource\EntityInterface;
 use Cake\Utility\Hash;
 use Cake\View\SerializedView;
+use Stringable;
 
 /**
  * A view class that is used for CSV responses.
@@ -77,11 +78,18 @@ class CsvView extends SerializedView
     protected string $subDir = 'csv';
 
     /**
-     * Whether or not to reset static variables in use
+     * Aggregated CSV output for the current serialization pass.
      *
-     * @var bool
+     * @var string
+     */
+    protected string $csv = '';
+
+    /**
+     * Temp stream used by fputcsv() to generate a single row.
+     *
+     * @var resource|null
      */
-    protected bool $_resetStaticVariables = false;
+    protected $fp = null;
 
     /**
      * Iconv extension.
@@ -201,16 +209,43 @@ public static function contentType(): string
      */
     protected function _serialize(array|string $serialize): string
     {
+        $this->resetState();
+
         $this->_renderRow($this->getConfig('header'));
         $this->_renderContent();
         $this->_renderRow($this->getConfig('footer'));
-        $content = $this->_renderRow();
-        $this->_resetStaticVariables = true;
-        $this->_renderRow();
+        $content = $this->csv;
+
+        $this->resetState();
 
         return $content;
     }
 
+    /**
+     * Reset accumulated state so the same view instance can render multiple
+     * times in a single request (queue worker, multi-file export, etc.).
+     */
+    protected function resetState(): void
+    {
+        $this->csv = '';
+        $this->isFirstBom = true;
+        if (is_resource($this->fp)) {
+            fclose($this->fp);
+        }
+        $this->fp = null;
+    }
+
+    /**
+     * @inheritDoc
+     */
+    public function __destruct()
+    {
+        if (is_resource($this->fp)) {
+            fclose($this->fp);
+            $this->fp = null;
+        }
+    }
+
     /**
      * Renders the body of the data to the csv
      *
@@ -245,24 +280,35 @@ protected function _renderContent(): void
                 foreach ($extract as $formatter) {
                     if (!is_string($formatter) && is_callable($formatter)) {
                         $value = $formatter($_data);
+                        $pathForError = '<callable>';
                     } else {
                         $path = $formatter;
                         $format = null;
                         if (is_array($formatter)) {
                             [$path, $format] = $formatter;
                         }
+                        $pathForError = (string)$path;
 
-                        if (!str_contains($path, '.')) {
-                            $value = $_data[$path];
-                        } else {
-                            $value = Hash::get($_data, $path);
-                        }
+                        $value = Hash::get($_data, $path);
 
-                        if ($format) {
+                        if ($format !== null) {
                             $value = sprintf($format, $value);
                         }
                     }
 
+                    if (
+                        $value !== null
+                        && !is_scalar($value)
+                        && !($value instanceof Stringable)
+                    ) {
+                        throw new CakeException(sprintf(
+                            'Extract path `%s` resolved to a non-scalar `%s`. '
+                            . 'Use a callable formatter to flatten it, or adjust the extract path.',
+                            $pathForError,
+                            get_debug_type($value),
+                        ));
+                    }
+
                     $values[] = $value;
                 }
                 $this->_renderRow($values);
@@ -273,54 +319,44 @@ protected function _renderContent(): void
     /**
      * Aggregates the rows into a single csv
      *
-     * @param array<string>|null $row Row data
+     * @param array<scalar|\Stringable|null>|null $row Row data
      * @return string CSV with all data to date
      */
     protected function _renderRow(?array $row = null): string
     {
-        static $csv = '';
+        $this->csv .= (string)$this->_generateRow($row);
 
-        if ($this->_resetStaticVariables) {
-            $csv = '';
-            $this->_resetStaticVariables = false;
-
-            return '';
-        }
-
-        $csv .= (string)$this->_generateRow($row);
-
-        return $csv;
+        return $this->csv;
     }
 
     /**
      * Generates a single row in a csv from an array of
      * data by writing the array to a temporary file and
      * returning its contents
      *
-     * @param array<string|null>|null $row Row data
+     * @param array<scalar|\Stringable|null>|null $row Row data
      * @return string|false String with the row in csv-syntax, false on fputscv failure
      */
     protected function _generateRow(?array $row = null): string|false
     {
-        static $fp = false;
-
         if (!$row) {
             return '';
         }
 
-        if ($fp === false) {
+        if ($this->fp === null) {
             $stream = 'php://temp';
             $fp = fopen($stream, 'r+');
             if ($fp === false) {
                 throw new CakeException(sprintf('Cannot open stream `%s`', $stream));
             }
+            $this->fp = $fp;
 
             $setSeparator = $this->getConfig('setSeparator');
             if ($setSeparator) {
-                fwrite($fp, 'sep=' . $setSeparator . "\n");
+                fwrite($this->fp, 'sep=' . $setSeparator . "\n");
             }
         } else {
-            ftruncate($fp, 0);
+            ftruncate($this->fp, 0);
         }
 
         $null = $this->getConfig('null');
@@ -340,21 +376,21 @@ protected function _generateRow(?array $row = null): string|false
         /** @phpstan-ignore-next-line */
         $row = str_replace(["\r\n", "\n", "\r"], $newline, $row);
         if ($enclosure === '') {
-            // fputcsv does not supports empty enclosure
-            if (fputs($fp, implode($delimiter, $row) . "\n") === false) {
+            // fputcsv does not support empty enclosure
+            if (fputs($this->fp, implode($delimiter, $row) . "\n") === false) {
                 return false;
             }
         } else {
-            if (fputcsv($fp, $row, $delimiter, $enclosure, $escape) === false) {
+            if (fputcsv($this->fp, $row, $delimiter, $enclosure, $escape) === false) {
                 return false;
             }
         }
 
-        rewind($fp);
+        rewind($this->fp);
         unset($row);
 
         $csv = '';
-        while (($buffer = fgets($fp, 4096)) !== false) {
+        while (($buffer = fgets($this->fp, 4096)) !== false) {
             $csv .= $buffer;
         }
 

tests/TestCase/View/CsvViewTest.php

@@ -3,6 +3,7 @@
 
 namespace CsvView\Test\TestCase\View;
 
+use Cake\Core\Exception\CakeException;
 use Cake\Http\Response;
 use Cake\Http\ServerRequest as Request;
 use Cake\I18n\DateTime;
@@ -519,4 +520,81 @@ public function testInvalidViewVarThrowsException()
         $this->view->setConfig('serialize', 'data');
         $this->view->render();
     }
+
+    /**
+     * Rendering the same instance twice must produce clean output both times
+     * (no stale BOM state, no leftover writer state).
+     *
+     * @return void
+     */
+    public function testRenderTwiceWithSameInstance()
+    {
+        $data = [['a', 'b'], ['c', 'd']];
+        $this->view->set(['data' => $data])
+            ->setConfig(['serialize' => 'data', 'bom' => true, 'csvEncoding' => 'UTF-8']);
+
+        $bom = chr(0xEF) . chr(0xBB) . chr(0xBF);
+        $expected = $bom . 'a,b' . PHP_EOL . 'c,d' . PHP_EOL;
+
+        $this->assertSame($expected, $this->view->render());
+        $this->assertSame($expected, $this->view->render());
+    }
+
+    /**
+     * Simple (non-dotted) extract paths must fall through Hash::get() so a
+     * missing key resolves to null instead of triggering an undefined-key
+     * warning.
+     *
+     * @return void
+     */
+    public function testRenderViaExtractMissingSimpleKey()
+    {
+        $data = [
+            ['name' => 'alice', 'email' => 'a@example.com'],
+            ['name' => 'bob'], // missing 'email'
+        ];
+        $this->view->set(['users' => $data])
+            ->setConfig([
+                'serialize' => 'users',
+                'extract' => ['name', 'email'],
+            ]);
+
+        $expected = 'alice,a@example.com' . PHP_EOL . 'bob,' . PHP_EOL;
+        $this->assertSame($expected, $this->view->render());
+    }
+
+    /**
+     * An extract path that resolves to an array (e.g. a hasMany association)
+     * must throw a clear exception instead of silently producing "Array to
+     * string conversion" notices and corrupted CSV. Regression for #131.
+     *
+     * @return void
+     */
+    public function testRenderViaExtractArrayValueThrows()
+    {
+        $data = [
+            [
+                'id' => 1,
+                'tags' => [['name' => 'php'], ['name' => 'cakephp']],
+            ],
+        ];
+        $this->view->set(['rows' => $data])
+            ->setConfig([
+                'serialize' => 'rows',
+                'extract' => ['id', 'tags'],
+            ]);
+
+        try {
+            $this->view->render();
+            $this->fail('Expected exception for array-valued extract path.');
+        } catch (Exception $e) {
+            // SerializedView wraps our CakeException in SerializationFailureException.
+            $previous = $e->getPrevious() ?? $e;
+            $this->assertInstanceOf(CakeException::class, $previous);
+            $this->assertStringContainsString(
+                'Extract path `tags` resolved to a non-scalar `array`',
+                $previous->getMessage(),
+            );
+        }
+    }
 }