Skip to content

Stop the callback expression parser corrupting and dropping arguments - #352

Open
sadiqk2 wants to merge 1 commit into
NativePHP:mainfrom
sadiqk2:fix/callback-argument-quoting
Open

Stop the callback expression parser corrupting and dropping arguments#352
sadiqk2 wants to merge 1 commit into
NativePHP:mainfrom
sadiqk2:fix/callback-argument-quoting

Conversation

@sadiqk2

@sadiqk2 sadiqk2 commented Aug 17, 2026

Copy link
Copy Markdown

The bug

CallbackRegistry::parse() converted argument literals to JSON with str_replace("'", '"'), which rewrites every apostrophe in the expression regardless of its role. Three distinct failures, none of which reports anything:

Expression Result
save("don't") arguments dropped — the apostrophe becomes a quote, the JSON is invalid
setName('O'Brien') arguments dropped
rename('it\'s fine') arguments corrupted to it"s fine

The third is the one worth being careful about: json_decode succeeds, 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 like save($draft) looks exactly like a call that worked.

The fix

The conversion now walks the string tracking which quote opened the current literal:

  • an apostrophe inside a double-quoted literal is data and stays put;
  • \' inside a single-quoted literal becomes a bare apostrophe, since JSON has no such escape;
  • 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 now says so via error_log instead 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 to main's baseline (Android splash-screen and release-build tests, unrelated). vendor/bin/pint clean.

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.
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.

1 participant