Stop the callback expression parser corrupting and dropping arguments - #352
Open
sadiqk2 wants to merge 1 commit into
Open
Stop the callback expression parser corrupting and dropping arguments#352sadiqk2 wants to merge 1 commit into
sadiqk2 wants to merge 1 commit into
Conversation
Converting argument literals to JSON was `str_replace("'", '"')`, which rewrote
every apostrophe in the expression whatever its role. Three failures came out of
that, all silent:
save("don't") arguments dropped entirely — invalid JSON
setName('O'Brien') arguments dropped entirely
rename('it\'s fine') arguments CORRUPTED to `it"s fine`
The third is the one worth fixing carefully: json_decode succeeds, the handler
runs, and it runs with a value the author never wrote. The first two invoke the
handler with no arguments at all, which for `save($draft)` is a call that looks
like it worked.
The conversion now tracks which quote opened the current string, so an apostrophe
inside a double-quoted literal is data and stays put, `\'` inside a single-quoted
one becomes a bare apostrophe (JSON has no such escape), and a double quote inside
a single-quoted literal is escaped rather than rewritten.
An expression that still cannot be parsed keeps the existing contract — `args =>
[]`, because callers spread the result and a null would be a TypeError — but it is
no longer silent about it.
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.
The bug
CallbackRegistry::parse()converted argument literals to JSON withstr_replace("'", '"'), which rewrites every apostrophe in the expression regardless of its role. Three distinct failures, none of which reports anything:save("don't")setName('O'Brien')rename('it\'s fine')it"s fineThe third is the one worth being careful about:
json_decodesucceeds, so the handler runs — with a value the author never wrote. The first two invoke the handler with no arguments at all, which for something likesave($draft)looks exactly like a call that worked.The fix
The conversion now walks the string tracking which quote opened the current literal:
\'inside a single-quoted literal becomes a bare apostrophe, since JSON has no such escape;An expression that still cannot be parsed keeps the existing contract —
args => [], because callers spread the result and anullwould be a TypeError — but it now says so viaerror_loginstead of failing silently. Changing that contract felt like it belongs in its own change rather than a bug fix.Verification
Six cases added to
tests/Unit/Edge/CallbackRegistryTest.php: apostrophe in a double-quoted argument, escaped apostrophe in a single-quoted one, double quote inside a single-quoted one, the ordinary literals (int, string, bool, null, multiple args, no parens, empty parens), a comma inside a quoted argument, and the unparseable fallback.Three fail against
main. Full suite: 887 passed, with the 13 failures identical tomain's baseline (Android splash-screen and release-build tests, unrelated).vendor/bin/pintclean.