Skip to content

Fix render-state leak and inconsistent extract path handling#146

Closed
dereuromark wants to merge 1 commit into
5.xfrom
bugfix-state-extract
Closed

Fix render-state leak and inconsistent extract path handling#146
dereuromark wants to merge 1 commit into
5.xfrom
bugfix-state-extract

Conversation

@dereuromark
Copy link
Copy Markdown
Member

Summary

Two correctness fixes plus a clearer error path. Tight scope; behavior changes are only in error paths (where the previous code silently corrupted output).

1. Render state leak across _serialize() calls

_renderRow() and _generateRow() kept their accumulator and temp stream in static function-scope variables. That meant:

  • The same view instance rendered twice in one request reused stale state (the temp php://temp handle survived, isFirstBom flipped after the first render so subsequent renders with bom => true silently emitted no BOM).
  • The reset path ($_resetStaticVariables) only zeroed the CSV string; it never closed or reopened the file handle, and it never reset isFirstBom.

Now both live on the instance, get reset at the start of every _serialize() call, and the handle is closed in __destruct() and via the explicit resetState() helper.

2. extract path handling was inconsistent

Simple (non-dotted) paths used $_data[$path] directly and emitted an undefined-key warning when the key was missing in some rows; dotted paths already went through Hash::get() and returned null cleanly. Both branches now go through Hash::get().

3. Clearer error when an extract value is non-scalar

When an extract path resolves to an array (e.g. a hasMany association, or any path that lands on a nested record), the previous code dropped the array into str_replace() / fputcsv() and produced "Array to string conversion" notices and corrupted CSV. We now throw a CakeException that names the offending path and the resolved type, so the user can either adjust the extract path or use a callable formatter to flatten the value. Closes #131.

Tests

  • testRenderTwiceWithSameInstance — render twice on the same instance with bom => true and assert both outputs include the BOM and the rows.
  • testRenderViaExtractMissingSimpleKey — missing simple key resolves to null (no notice, empty cell).
  • testRenderViaExtractArrayValueThrows — extract path that resolves to an array throws a CakeException with a helpful message (wrapped by SerializationFailureException from SerializedView).

All three gates green locally on PHP 8.4: phpunit (20/20), phpstan (level 8, clean), phpcs (clean).

Out of scope (follow-ups planned)

  • PHP 8.4 fputcsv($escape) deprecation (the current default '\\' will keep warning on PHP 8.4) — separate PR; switching the default to '' is RFC-4180-compliant but is a small output change for fields containing backslash, deserves its own CHANGELOG note.
  • Silent iconv() failure path (returns false, currently cast to '').
  • Streaming render mode for issue Outputting large amounts of rows #89.

- Convert the per-render static `$csv` / `$fp` locals in
  `_renderRow` / `_generateRow` to instance properties and reset
  them (including `isFirstBom`) at the start of every `_serialize()`
  call. This makes the same view instance safely reusable across
  renders (queue workers, multi-file exports) where the BOM was
  previously emitted only once and the temp stream carried stale
  state between renders.
- Drop the awkward `_resetStaticVariables` flag and the trailing
  `_renderRow()` reset call in `_serialize()`.
- Close the temp stream in `__destruct()` and via the explicit
  `resetState()` helper.
- Route all extract paths through `Hash::get()`. Previously simple
  (non-dotted) keys went through `$_data[$path]` and raised an
  undefined-key warning when the key was missing in some rows;
  dotted paths already returned null via Hash::get.
- Throw a clear `CakeException` when an extract path resolves to
  an array or non-Stringable object instead of silently producing
  "Array to string conversion" notices and corrupted output.

Adds regression tests for: repeat render with BOM on the same
instance, simple extract path with missing key, and array-valued
extract path producing a helpful error.
@dereuromark
Copy link
Copy Markdown
Member Author

Heads-up on the failing cs-stan check: the failure is not caused by this PR. It reproduces on 5.x if you run composer update today — squizlabs/php_codesniffer recently hit v4 which is incompatible with the sniffs that slevomat/coding-standard 8.29 references via cakephp/cakephp-codesniffer 5.3. Locally with the previously-locked versions (codesniffer 3.12, slevomat 8.17), phpcs is clean for this PR.

The fix is upstream / a separate composer pin (e.g. squizlabs/php_codesniffer: ~3.13 in cakephp-codesniffer's require), not in this PR. Test suite passes across PHP 8.2–8.5 and mysql/pgsql/sqlite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Export data from query with related table

1 participant