Fix invalid JSON dump for objects without debug properties - #142
Open
ousamabenyounes wants to merge 1 commit into
Open
Fix invalid JSON dump for objects without debug properties#142ousamabenyounes wants to merge 1 commit into
ousamabenyounes wants to merge 1 commit into
Conversation
meminfo_dump() always wrote a comma after "object_handle", but the "children" block meant to follow it is only emitted when the object's debug handler exposes properties. FFI\CType exposes none, so such a dump ended on a dangling comma and json_decode() rejected the whole file. Emit the separator from the branch that actually writes children, and a plain newline otherwise, mirroring the scalar branch below. Generated by Ora Studio Vibe coded by ousamabenyounes Co-Authored-By: Ora Agent <noreply@oratelecom.net>
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.
Fix #104
Summary
meminfo_dump()writes"object_handle" : "N",with an unconditional trailingcomma, then only writes the
"children"block when the object exposes debugproperties. An object whose debug handler exposes nothing therefore ends on a
dangling comma and the whole dump stops being parseable:
That is exactly the output pasted in the issue, and
json_decode()on the dumpreturns
JSON_ERROR_SYNTAX, so the analyzer cannot load the file at all.Root cause
meminfo_zval_dump()resolves properties withzend_get_properties_for(zv, ZEND_PROP_PURPOSE_DEBUG). That purpose supersedesthe
get_debug_infohandler, and a handler is allowed to returnNULL—FFI\CTypedoes exactly that. The separator was written before knowing whetheranything would follow it.
The fix writes
"object_handle"without a separator and lets each branch emitits own:
,\nwhen the"children"block follows,\nwhen it does not. Thismirrors the existing scalar branch a few lines below, so the emitted bytes are
unchanged for every object that does expose properties.
The
NULLcase is already the documented shape on the reader side — everyanalyzer consumer guards with
isset($item['children'])— so nothing else hasto change.
Test verification (RED → GREEN)
New test:
extension/tests/bug-github-104_ffi_object_invalid_json.phpt, in theexisting
dump-check_json*.phptidiom. ItSKIPIFs without ext/FFI (which onlyexists from PHP 7.4 on).
RED — unmodified
master(0ab7f5a), new test applied on its own, no production change:with the expected-vs-actual diff:
GREEN — this branch, full
.phptsuite on PHP 8.1:A third run with the production hunk reverted but the test kept still fails, so
the test is tied to the fix and not to the environment.
Full local suite
The complete local validation suite was replayed in Docker — every job class of
.github/workflows/build.yaml, on the whole matrix(
phpize && ./configure && make && make test)(PHP 7.0, 7.1, 7.2, 7.3, 7.4, 8.0, 8.1) plus
cd analyzer && composer install && vendor/bin/phpspec run..phptsuiteOn
masterthe same replay is 13 passed / 0 failed on every version, so nopreviously green test changed state and the passing count never drops.
Line coverage of the changed C lines, measured with
gcovover the.phptsuite:
100.00% (3/3) added executable lines covered.Note on CI: this repo's workflow only triggers
on: pushand pins the retiredubuntu-20.04runner, so pull requests here get an empty check list — that ispre-existing and not specific to this PR, which is why the replay above is run
locally.
Note for reviewers
Open PR #136 touches the immediately adjacent lines (it changes which
properties are fetched for the dump). It is not a duplicate of this one — it
still needs this
NULLguard — but whichever lands second will need a smalltextual rebase.
Files changed
extension/meminfo.c"object_handle"separator only when a"children"block actually followsextension/tests/bug-github-104_ffi_object_invalid_json.phptFFI\CTypemust produce decodable JSON