Fix render-state leak and inconsistent extract path handling#146
Closed
dereuromark wants to merge 1 commit into
Closed
Fix render-state leak and inconsistent extract path handling#146dereuromark wants to merge 1 commit into
dereuromark wants to merge 1 commit into
Conversation
- 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.
Member
Author
|
Heads-up on the failing The fix is upstream / a separate composer pin (e.g. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 instaticfunction-scope variables. That meant:php://temphandle survived,isFirstBomflipped after the first render so subsequent renders withbom => truesilently emitted no BOM).$_resetStaticVariables) only zeroed the CSV string; it never closed or reopened the file handle, and it never resetisFirstBom.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 explicitresetState()helper.2.
extractpath handling was inconsistentSimple (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 throughHash::get()and returnednullcleanly. Both branches now go throughHash::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 aCakeExceptionthat 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 withbom => trueand assert both outputs include the BOM and the rows.testRenderViaExtractMissingSimpleKey— missing simple key resolves tonull(no notice, empty cell).testRenderViaExtractArrayValueThrows— extract path that resolves to an array throws aCakeExceptionwith a helpful message (wrapped bySerializationFailureExceptionfromSerializedView).All three gates green locally on PHP 8.4:
phpunit(20/20),phpstan(level 8, clean),phpcs(clean).Out of scope (follow-ups planned)
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.iconv()failure path (returnsfalse, currently cast to'').