Repository navigation
Conversation
Since the bulk string scan from #1948, the Lexer accepts invalid UTF-8 inside strings and keeps the raw bytes. Comments and block strings swallow up to three bytes after an invalid lead byte. The spec only allows Unicode scalar values as SourceCharacter. 🤖 Generated with Claude Code
🤖 Generated with Claude Code
spawnia
added a commit
that referenced
this pull request
Oct 3, 2026
Token positions count characters the way Lexer::readChar() does, so stepping by lead byte finds the byte offsets without converting the whole body. Relies on #1987 rejecting invalid UTF-8. 🤖 Generated with Claude Code
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Error columns are incorrect when a multibyte character precedes a newline before the invalid byte.
Review effort: Balanced
Findings: 1
What changed in this PR
Rejects malformed UTF-8 before lexing to prevent invalid AST values.
Changes:
- Validates complete documents and reports the first invalid byte.
- Adds malformed UTF-8 regression tests.
- Documents the fix.
| File | Description |
|---|---|
src/Language/Lexer.php |
Adds document-wide UTF-8 validation. |
tests/Language/LexerTest.php |
Tests invalid encodings and locations. |
CHANGELOG.md |
Records the fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| $validPrefix = $matches[0] ?? ''; | ||
| $invalidByte = strtoupper(bin2hex($body[strlen($validPrefix)])); | ||
|
|
||
| throw new SyntaxError($this->source, mb_strlen($validPrefix, 'UTF-8'), "Invalid UTF-8 byte: 0x{$invalidByte}"); |
Collaborator
Author
There was a problem hiding this comment.
The cause is older than this pull request: Source::getLocation() gives wrong columns for every syntax error after a multibyte line, e.g. # äää\nxx? reports 2:-2 on master. Fixed separately in #1988, after which I will add your case to this provider.
The prefix regex hit pcre.backtrack_limit on about 1 million 3- or 4-byte characters and then reported the first byte of the document. mb_scrub() replaces only invalid bytes, so the first differing byte is the invalid one. 🤖 Generated with Claude Code
spawnia
added a commit
that referenced
this pull request
Oct 6, 2026
Resolves #1028 graphql-php has no equivalent of graphql-js `stripIgnoredCharacters`, which clients use to send compact documents. This adds `Printer::stripIgnoredCharacters()`, a port of https://github.com/graphql/graphql-js/blob/17.x.x/src/utilities/stripIgnoredCharacters.ts. <details> <summary>Merge #1987 first, its commits show in this diff until then</summary> String tokens are copied byte for byte, as in graphql-js. Token positions count characters, so the byte offsets come from stepping lead bytes like `Lexer::readChar()`. That matches the Lexer only for valid UTF-8, which #1987 enforces. GitHub refused to change the base branch, so this pull request targets master. </details> <details> <summary>Parsing the output gives the same AST as parsing the input, apart from locations</summary> Block strings are re-printed minimized, through a new `$minimize` flag on `BlockString::print()`. Both `stripIgnoredCharacters-test.ts` and `stripIgnoredCharacters-fuzz.ts` are ported, plus the minimized cases from `blockString-test.ts`. The fuzz suite checks that every block string up to 7 characters, built from 6 tricky characters, keeps its value. </details> <details> <summary>Runtime stays linear in document size</summary> On a 664 KB document, stripping takes 0.85s and `Parser::parse()` takes 1.07s. Slicing with `mb_substr()` instead took 3.85s, because it scans from the start of the body for every string token. </details> 🤖 Generated with Claude Code
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 3a97f1b | Previous: 32edc67 | Ratio |
|---|---|---|---|
BuildSchemaBench::benchBuildSchema |
25.187 ms |
13.587 ms |
1.85 |
OverlappingFieldsCanBeMergedBench::benchRepeatedFields100 |
3.276 ms |
1.748 ms |
1.87 |
OverlappingFieldsCanBeMergedBench::benchRepeatedFields500 |
15.55 ms |
8.304 ms |
1.87 |
OverlappingFieldsCanBeMergedBench::benchRepeatedFields1000 |
30.983 ms |
16.665 ms |
1.86 |
OverlappingFieldsCanBeMergedBench::benchRepeatedFields2000 |
63.005 ms |
35.324 ms |
1.78 |
OverlappingFieldsCanBeMergedBench::benchRepeatedFields3000 |
95.55 ms |
52.762 ms |
1.81 |
VisitorBench::benchVisitIntrospectionWithEnterLeave |
0.308 ms |
0.16 ms |
1.93 |
VisitorBench::benchVisitIntrospectionWithKindMap |
0.305 ms |
0.158 ms |
1.93 |
VisitorBench::benchVisitIntrospectionWithKindCallable |
0.28 ms |
0.146 ms |
1.92 |
VisitorBench::benchVisitIntrospectionWithEnterLeaveMap |
0.307 ms |
0.159 ms |
1.93 |
VisitorBench::benchVisitNestedWithEnterLeave |
0.071 ms |
0.037 ms |
1.92 |
ScalarOverrideBench::benchExecuteWithoutOverride |
0.163 ms |
0.082 ms |
1.99 |
ScalarOverrideBench::benchExecuteWithTypesOverride |
0.162 ms |
0.081 ms |
2 |
StarWarsBench::benchSchema |
0.005 ms |
0.003 ms |
1.67 |
StarWarsBench::benchHeroQuery |
0.328 ms |
0.175 ms |
1.87 |
StarWarsBench::benchNestedQuery |
0.715 ms |
0.388 ms |
1.84 |
StarWarsBench::benchQueryWithFragment |
0.765 ms |
0.412 ms |
1.86 |
StarWarsBench::benchQueryWithInterfaceFragment |
0.724 ms |
0.388 ms |
1.87 |
StarWarsBench::benchStarWarsIntrospectionQuery |
7.22 ms |
3.843 ms |
1.88 |
HugeSchemaBench::benchSchema |
13.05 ms |
7.137 ms |
1.83 |
HugeSchemaBench::benchSmallQuery |
14.675 ms |
8.038 ms |
1.83 |
HugeSchemaBench::benchSmallQueryLazy |
15.62 ms |
8.992 ms |
1.74 |
LexerBench::benchIntrospectionQuery |
0.3 ms |
0.165 ms |
1.82 |
LexerBench::benchDeeplyIndentedQuery |
1.926 ms |
1.09 ms |
1.77 |
DeferredBench::benchSingleDeferred |
0.001 ms |
0 ms |
+∞ |
DeferredBench::benchNestedDeferred |
0.003 ms |
0.001 ms |
3 |
DeferredBench::benchChain5 |
0.005 ms |
0.003 ms |
1.67 |
DeferredBench::benchChain100 |
0.081 ms |
0.043 ms |
1.88 |
DeferredBench::benchManyDeferreds |
0.449 ms |
0.227 ms |
1.98 |
DeferredBench::benchManyNestedDeferreds |
12.526 ms |
6.202 ms |
2.02 |
DeferredBench::bench1000Chains |
3.429 ms |
1.74 ms |
1.97 |
This comment was automatically generated by workflow using github-action-benchmark.
This branch has not been deployed
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.
Since v15.36.0,
Parser::parseaccepts invalid UTF-8 inside strings.The resulting values make
Printer::doPrintandjson_encodefail.The Lexer now checks each document once and throws
Syntax Error: Invalid UTF-8 byte: 0xFFat the first invalid byte.Regression from https://github.com//pull/1948
v15.35.0 rejected
{ a(b: "\xffabc") }, although with the misleadingCannot contain the invalid character <EOF>.The bulk
strcspn()scan inreadString()skips per-character decoding, so v15.36.0 keeps\xffin the value.Printer::doPrint()on that AST throwsJsonException: Malformed UTF-8 characters.Comments and block strings mishandled invalid bytes before that
Lexer::readChar()takes the byte length from the lead byte without checking continuation bytes.# \xffswallows the following newline, and"""\xff"""swallows the closing quotes.Both end in misleading errors.
The spec allows only Unicode scalar values
https://spec.graphql.org/October2021/#SourceCharacter excludes surrogates, and graphql-js rejects them with
Invalid character within String.Overlong encodings and encoded surrogates are rejected too, as
mb_check_encoding()does.Parsing speed is unchanged
mb_check_encoding()runs once per document.Best of 5 runs,
Parser::parsewithout locations:Only invalid input pays for locating the byte.
mb_scrub()replaces invalid bytes, and the first byte that differs is the one reported.For a 4 MB document with the invalid byte at the end, that takes 0.02s.
Copilot's column case follows after https://github.com//pull/1988
#1987 (comment) found wrong columns after multibyte lines.
Source::getLocation()causes them for every syntax error, which #1988 fixes.Once that is merged, the case
# ä\nxx\xFFgoes into this provider.🤖 Generated with Claude Code
Changelog
Fixed