feat(cmap): ToUnicode CMap + text-space matching (PR #06, stacked on #7)#8
Conversation
…ument (SAPP PR #6) The largest single PR in the text-replacement series. Refactors PDFDoc::replaceTextInDocument from byte-space to text-space matching so it works on Identity-H subset fonts (the >95% Word-generated Woo PDF case). Adds two new top-level classes and 8 helper methods. New classes: src/CMap.php (~240 LOC) Parser + lookup tables for ToUnicode CMap streams per Adobe Tech Note 5411. Handles beginbfchar/endbfchar blocks and both beginbfrange/endbfrange shapes (contiguous + array). Multi- codepoint Unicode targets (ligatures, decomposed accents) NFC-normalised on insert. Unsupported directives silently ignored. src/FontEncoding.php (~170 LOC) Implicit-encoding tables for simple fonts. /WinAnsiEncoding (full PDF 1.7 Appendix D Table D.2), /MacRomanEncoding (common Western European subset), /StandardEncoding (ASCII passthrough), /Identity-H / /Identity-V (discriminators only; require a CMap). PDFDoc changes: - buildFontContext / collectPageFonts / buildFontInfo: walk the /Catalog/Pages tree to map each page's /Resources/Font entries to (encoding, cmap, baseFont) tuples; index by content-stream OID so each stream's processing knows its font set. - replaceInContentStream: linear-scan operator walker that tracks active font from Tf, parses Tj operand (both (literal) and <hex> shapes), resolves operand to Unicode via active font, runs substitutions in text space, splices placeholder back. - findNextOperator / findTjOperand: token-aware operator locators (whitespace-bounded). - resolveOperandToUnicode / encodeUnicodeViaFont: forward + reverse map dispatch through CMap (preferred) → FontEncoding (fallback) → Latin-1 passthrough (when no font info). - spliceOperand: locates needle in resolved text, translates back to byte offsets via per-CID accounting, detects cross-CID- boundary matches (records cid_split_mismatch + skips). - escapePdfLiteral / unescapePdfLiteral: PostScript-style escape handling for (literal) Tj operand round-trip. Diagnostic surface grew additively: - font_encoding_misses: array<oid, array<needle, baseFont>> Records substitutions skipped because the active font's forward map can't encode every placeholder character. PR #8 adds the Helvetica fallback that recovers from this. - cid_split_mismatch: array<oid, array<needle, byte_offset>> Records substitutions skipped because the match boundary would split a multi-codepoint CID (e.g. matching "f" against a "fi" ligature CID). Backwards compatibility: WinAnsi-encoded literal-string Tj operators (the existing PoC fixture path) round-trip byte-for-byte identically — WinAnsi's printable ASCII range maps each byte to its own codepoint, so text-space matching lands at the same byte offsets as the prior byte-space PoC. poc-replace-text.php exits 0 with the same diagnostic shape (streams_modified=1, residual_needles=0). PHP gotcha caught during testing: `mb_convert_encoding(..., 'UTF-8', 'UTF-32BE')` returns the STRING '0' for codepoint 48. The `?:` ternary substituted '' for that case because '0' is falsy in PHP. Fixed with explicit `=== false` check in both FontEncoding::byteToUnicode and CMap::codepointToUnicode. Worth surfacing in upstream review. Contract pinned by openspec/changes/feat-tounicode-cmap/ (proposal + design D1-D6 + spec REQs across two capabilities: tounicode-cmap-resolution and a MODIFIED text-replacement capability + tasks). Verification: - examples/poc-tounicode-cmap.php (new) — 5 REQs covered: CMap bfchar / bfrange-contiguous / bfrange-array / multi- codepoint target; FontEncoding WinAnsi printable-ASCII round- trip + Euro 0x80; existing PoC verify gate; forward-map placeholder encoding; unencodable-character null return. - All 6 prior gates remain green (PoC + dispatcher + 4 filter round-trips) after the major PDFDoc refactor. Closes openspec/changes/feat-tounicode-cmap/tasks.md 52/52.
| } | ||
| return $str; | ||
| } | ||
| } |
There was a problem hiding this comment.
(Anchored to nearest diff line src/CMap.php:290; finding concerns src/CMap.php:708.)
🟡 Concern — Regex-based CMap parser
parse() strips %-to-EOL comments globally, then runs preg_match_all('/beginbfchar\s+(.*?)\s+endbfchar/s'). Works for Word-emitted shape but fails on CMaps where a comment falls between hex pairs (stripped to bare \n OK), and a stream containing endbfchar substring inside a string/comment terminates the block prematurely. Also: %-stripper only matches %[^\n]*\n — comments at EOF without trailing newline aren't stripped.
Suggested fix: Use a proper PostScript tokenizer or scope the regex.
There was a problem hiding this comment.
Still uses regex line-strip for comments — preg_replace('/%[^\n]*\n/', "\n", $bytes) is unchanged. Real impact is low for beginbfchar/beginbfrange-only CMaps (hex digits don't contain '%'), but a CMap with '%' in a name or other PostScript construct would still mis-parse. Mark as known-limitation in the CMap class docblock if you want to defer, or implement a state-aware tokenizer for a future pass.
|
|
||
| /** | ||
| * This functions outputs the document to a buffer object, ready to be dumped to a file. | ||
| * @param rebuild whether we are rebuilding the whole xref table or not (in case of incremental versions, we should use "false") |
There was a problem hiding this comment.
(Anchored to nearest diff line src/PDFDoc.php:1477; finding concerns src/PDFDoc.php:1696.)
🟡 Concern — Forward-map lookup misses multi-codepoint keys
encodeUnicodeViaFont() uses preg_split('//u', $unicode, -1, PREG_SPLIT_NO_EMPTY) yielding ONE codepoint per element. The CMap's forward map stores 'fi' => <CID> for ligature reverse-mappings, but per-codepoint walk looks up 'f' and 'i' separately.
Suggested fix: Greedy-match the forward map (longest-prefix-first).
There was a problem hiding this comment.
Unresolved. CMap::addMapping stores the full multi-codepoint $unicodeStr (e.g. "fi") as the forward-map key, but encodeUnicodeViaFont walks codepoint-by-codepoint via preg_split('//u', ...) and calls unicodeToCid($ch) with ONE codepoint at a time — a ligature key like "fi" can never be hit. Either change the encode path to greedy-prefix-match across the forward map, or document this as a known limitation in the CMap class docblock.
| * | ||
| * @param string $bytes Raw bytes. | ||
| * | ||
| * @return string Escaped literal (without surrounding parens). |
There was a problem hiding this comment.
🟡 Concern — Inline /Contents not handled
buildFontContext() only handles /Contents when it's an indirect reference. A page with an inline stream as /Contents (rare but spec-legal) yields no entry in streamToFonts, falls back to allFonts.
Suggested fix: Handle inline-stream /Contents shape.
There was a problem hiding this comment.
Inline /Contents (direct stream value, not an indirect reference) is still not handled — buildFontContext only walks values that resolve via get_object_referenced(). Add at least a contents_inline_pages diagnostic so callers can detect when a page's text content is skipped entirely (matches the diagnostic-surface pattern from Finding 8's encoding_dict_unhandled and Finding 9's contents_array_pages).
| $width = max(1, $cmap->cidWidth() ?: 2); | ||
| for ($i = 0; $i + $width <= $len; $i += $width) { | ||
| $cid = substr($bytes, $i, $width); | ||
| $u = $cmap->cidToUnicode($cid); |
There was a problem hiding this comment.
🟡 Concern — Silent allFonts fallback
$pageFonts = $fontContext['streamToFonts'][$oid] ?? $fontContext['allFonts']; — falling back to global merged font set is a debugging convenience. In a real document with multiple pages using different /F1 → font mappings, this applies the WRONG font's encoding/CMap to streams whose page-resolution failed.
Suggested fix: Make fallback empty (skip the stream) or record a page_resolution_misses diagnostic.
There was a problem hiding this comment.
Silent allFonts fallback unchanged — $pageFonts = $fontContext['streamToFonts'][$oid] ?? $fontContext['allFonts'] with no diagnostic. Add a pagefonts_fallback_oids diagnostic so callers know the per-page font-set mapping degraded to global merge. Same one-line pattern as the other diagnostics added in this commit.
| if ($cmap === null) return null; | ||
| $cid = $cmap->unicodeToCid($ch); | ||
| if ($cid === null) return null; | ||
| $out .= $cid; |
There was a problem hiding this comment.
🟡 Concern — Performance hot path
replaceInContentStream runs findNextOperator($stream, $pos, 'Tj') AND findNextOperator($stream, $pos, 'Tf') each iteration — each a full strpos from $pos to end. For N text operators in a 100KB-1MB stream: O(N · L).
Suggested fix: Cache next-operator positions or use a single forward-scan tokenizer.
There was a problem hiding this comment.
Still O(n²). And the new string-state-aware findNextOperator tokeniser (correctness fix for Finding 1) MAGNIFIES the per-iteration cost — each outer iteration runs the tokeniser twice (once for 'Tj', once for 'Tf'), and each call is a byte-walk through (...) literals and <...> hex strings. Refactor: single forward-pass tokeniser yielding (op, pos) tuples; consume the stream once with cursor state. Acceptable to defer in this PR — please file a follow-up issue.
|
|
||
| /** | ||
| * This functions outputs the document to a buffer object, ready to be dumped to a file. | ||
| * @param rebuild whether we are rebuilding the whole xref table or not (in case of incremental versions, we should use "false") |
There was a problem hiding this comment.
(Anchored to nearest diff line src/PDFDoc.php:1477; finding concerns src/PDFDoc.php:1750.)
🟡 **Concern — Two parallel resolution walks
spliceOperand builds $textBuffer + $byteOffsets once, then walks the operand a SECOND time re-resolving each CID to compute cidStart/cidEnd. A future change to the resolution chain in one walk and not the other silently corrupts byte-offset translation.
Suggested fix: Compute (cidStart, cidEnd) during the first walk.
There was a problem hiding this comment.
Still duplicated. spliceOperand walks the operand twice (once to build $textBuffer+$byteOffsets, again to compute $startByte/$endByte), and resolveOperandToUnicode has the same cmap/encoding fallback chain a third time. Extract a resolveSingleCid($cidBytes, $cmap, $encoding, $isIdentity): string helper and have all three sites call it. Doesn't change behaviour but eliminates the drift risk between three near-identical chains.
| foreach ($failures as $msg) { | ||
| echo " - $msg\n"; | ||
| } | ||
| exit(1); |
There was a problem hiding this comment.
(Anchored to nearest diff line examples/poc-tounicode-cmap.php:205; finding concerns examples/poc-tounicode-cmap.php:384.)
🟡 Concern — Test gate verifies exit code only
REQ-3 verifies the existing PoC 'still exits 0' by exec-ing poc-replace-text.php. But the backwards-compat claim is about the diagnostic surface, not just exit code.
Suggested fix: Capture $output and assert the diagnostic line matches.
There was a problem hiding this comment.
PoC gate still validates exit code only (if ($exit !== 0)). To catch silent regressions of substitution semantics, parse the child PoC's stdout (it already prints stats) and assert replacements_per_needle=1, unmatched_needles=[], font_encoding_misses=[]. Otherwise a regression that quietly stops replacing text would still exit 0.
| } | ||
| return $table; | ||
| } | ||
| } |
There was a problem hiding this comment.
(Anchored to nearest diff line src/FontEncoding.php:220; finding concerns src/FontEncoding.php:946.)
🟡 Concern — Public constructor undermines value-object intent
Public __construct($name, array $byteToCp = []) accepts arbitrary byte→codepoint table. PHPDoc hints at /Differences-overridden encodings but PR says NOT implemented.
Suggested fix: Make constructor private and route through forName, OR actually wire Differences-handling.
There was a problem hiding this comment.
Constructor still public. Change to private and route /Differences overrides through a withDifferences(array $overrides): self builder if /Differences support lands later. Keeps the value-object invariant that the static forName() factory was meant to enforce.
| } | ||
| return $table; | ||
| } | ||
| } |
There was a problem hiding this comment.
(Anchored to nearest diff line src/FontEncoding.php:220; finding concerns src/FontEncoding.php:1075.)
🟢 Minor — MacRoman table holes
Comment says 'partial coverage; expand as needed'. Returns encoding with HOLES in 0x80-0xFF range. byteToUnicode on a hole returns '', simple-font resolver re-interprets as Latin-1 passthrough — producing wrong characters.
Suggested fix: Document or add debug log when unknown MacRoman byte encountered.
There was a problem hiding this comment.
MacRoman table still partial — missing 0x98-0xA8, 0xAB-0xC7, 0xCA-0xCF, 0xD4-0xFF. Either complete it per PDF 1.7 Appendix D Table D.4, or document the limitation in the class docblock so callers know unmapped MacRoman bytes hit the Latin-1 fallback silently.
|
|
||
| /** | ||
| * This functions outputs the document to a buffer object, ready to be dumped to a file. | ||
| * @param rebuild whether we are rebuilding the whole xref table or not (in case of incremental versions, we should use "false") |
There was a problem hiding this comment.
(Anchored to nearest diff line src/PDFDoc.php:1477; finding concerns src/PDFDoc.php:1632.)
🟢 **Minor — DRY violation
Both methods walk operand bytes, look up CMap, fall back to encoding.byteToUnicode, fall back to Latin-1. Same chain implemented twice.
Suggested fix: Extract to single resolveOneCid(...) helper.
There was a problem hiding this comment.
Same code as the 'two parallel resolution walks' thread above — extracting a resolveSingleCid($cidBytes, $cmap, $encoding, $isIdentity): string helper would close both threads at once.
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Strict-mode review (REQUEST_CHANGES)
Heavy refactor introduces CMap parsing + text-space matching with two clean new value-object classes, but the operator scanner walks the raw stream without respecting string-literal boundaries, the active-font Tf back-scan uses a fragile 64-byte preg_match window, the diagnostic surface silently drops the documented unmatched_needles semantic, replacements_per_needle is incremented even on cid_split_mismatch (double-counting), multi-occurrence needles in one operand are not all replaced, and the page-tree walker ignores PDF-mandated /Resources inheritance even though task 4.3 is checked off.
Findings: 🔴 9 blockers · 🟡 17 concerns · 🟢 6 minors. All findings have been posted as inline comments above. Per Strict-mode rules, any 🟡 or 🔴 holds the verdict; address each thread before requesting re-review.
🔴 Blockers
- src/PDFDoc.php:1477 — Operator scanner ignores string-literal boundaries — Tj/Tf inside
(...)will be detected as operators - src/PDFDoc.php:1197 —
unmatched_needlesdocumented in the surface but never populated — backwards-compat claim broken - src/PDFDoc.php:1366 —
replacements_per_needleincremented even on cid_split_mismatch — double-counts skipped matches - src/PDFDoc.php:1314 — Active-font Tf back-scan uses 64-byte preg_match window starting AT operator — can mis-attribute font names
- src/PDFDoc.php:1477 — Multiple occurrences of the same needle in one operand are NOT all replaced
- src/PDFDoc.php:1452 —
/Resourcesinheritance up the /Pages tree is NOT implemented despite task 4.3 being checked off - src/PDFDoc.php:1477 — Backslash-escape handling in
findTjOperandliteral-string back-scan is broken for\\)sequences - src/PDFDoc.php:1477 — Custom
/Encodingdictionaries with/Differencessilently produce garbage instead of recording a miss - src/PDFDoc.php:1282 — Graphics-state Tf doesn't persist across /Contents array entries — Identity-H pages with multi-stream /Contents break
(Inline comments include **Impact:** and **Suggested fix:** sections per Strict-mode body template.)
Addresses all 6 blockers + 11 concerns + 4 minors on PR #1. 🔴 Blockers - poc-make-fixture.php: PHP 7.4 compat — replaced PHP 8 named args with positional. Helper renamed to snake_case (poc_build_content_stream, poc_obj) so the namespace+trailing-marker concerns get resolved together; namespace declaration dropped (PoC scripts shouldn't pollute ddn\sapp\*). [r3316596051] - PDFDoc::replaceTextInDocument → replace_text_in_document — match sapp's snake_case convention. The PoC's only caller is the verification script, so blast radius is contained. [r3316596182] - Placeholder constraint documented in the method docblock: callers MUST NOT pass `(`, `)`, or `\` in substitution values; the PoC does not escape PDF-string literals. Upstream PR #8 promotes this to runtime parameter validation (`rejected_substitutions`). [r3316596352] - streams_scanned semantics fixed: now counts every /FlateDecode stream visited (regardless of BT/ET), and a new content_streams_scanned key counts the BT-filtered subset. The docblock matches the contract. [r3316596522] - Write-back now uses add_object() with generation-precedence preservation; direct $_pdf_objects[$oid] = $obj clobber removed. A pre-existing higher-generation entry on the same OID is cloned forward instead of being silently demoted. [r3316596673] - Serialisation contract documented: callers MUST use to_pdf_file_b(true) / to_pdf_file_s(true). Incremental serialisation is broken once _pdf_objects is populated by the mutator — explicit in the docblock. [r3316596815] 🟡 Concerns - Filter equality: /Fl abbreviation and array-form chained filters are spec-legal but explicitly skipped — documented in the contract block. - $decoded === false guard broadened to !is_string($decoded) — survives a future refactor that returns null on failure. - Phase-2 sweep rewritten to walk every object and opportunistically gzuncompress raw streams — no longer replicates the production method's filter/BT skip heuristic, so a regression that tightens those skips can't pass vacuously. - Loop wrapped in push_state/try/catch so partial mid-loop failure rolls back via pop_state instead of leaving _pdf_objects in a half-mutated state. - poc-make-fixture.php: gzcompress level pinned to 6 (deterministic across zlib versions); explicit false-return check; missing trailing newline added. - poc-replace-text.php: now passes both a matching and an unmatched needle so unmatched_needles + replacements_per_needle are exercised end-to-end. Missing trailing newline added. - replace_text_in_document: switched to single-pass array-form str_replace + per-needle substr_count pre-pass — faster on multi-substitution batches over large streams. - poc-make-fixture: namespace declaration dropped (concerns paired with the trailing-marker minor). 🟢 Minors - PSR-12 trailing newline on both PoC scripts. - @return docblock now uses array-shape syntax for type-safe destructuring. - .gitignore: docs/.idea/ → /.idea/ (JetBrains creates the dir at project root, not under docs/). - Trailing-marker comments dropped from poc-make-fixture (sapp's other source files don't use them). Verification: examples/poc-replace-text.php exits 0 with the new mixed substitution set; diagnostic surface populates unmatched_needles correctly; Phase-2 exhaustive sweep finds 0 residual needles and 1 placeholder hit.
Addresses all 6 blockers + 11 concerns + 4 minors on PR #1. 🔴 Blockers - poc-make-fixture.php: PHP 7.4 compat — replaced PHP 8 named args with positional. Helper renamed to snake_case (poc_build_content_stream, poc_obj) so the namespace+trailing-marker concerns get resolved together; namespace declaration dropped (PoC scripts shouldn't pollute ddn\sapp\*). [r3316596051] - PDFDoc::replaceTextInDocument → replace_text_in_document — match sapp's snake_case convention. The PoC's only caller is the verification script, so blast radius is contained. [r3316596182] - Placeholder constraint documented in the method docblock: callers MUST NOT pass `(`, `)`, or `\` in substitution values; the PoC does not escape PDF-string literals. Upstream PR #8 promotes this to runtime parameter validation (`rejected_substitutions`). [r3316596352] - streams_scanned semantics fixed: now counts every /FlateDecode stream visited (regardless of BT/ET), and a new content_streams_scanned key counts the BT-filtered subset. The docblock matches the contract. [r3316596522] - Write-back now uses add_object() with generation-precedence preservation; direct $_pdf_objects[$oid] = $obj clobber removed. A pre-existing higher-generation entry on the same OID is cloned forward instead of being silently demoted. [r3316596673] - Serialisation contract documented: callers MUST use to_pdf_file_b(true) / to_pdf_file_s(true). Incremental serialisation is broken once _pdf_objects is populated by the mutator — explicit in the docblock. [r3316596815] 🟡 Concerns - Filter equality: /Fl abbreviation and array-form chained filters are spec-legal but explicitly skipped — documented in the contract block. - $decoded === false guard broadened to !is_string($decoded) — survives a future refactor that returns null on failure. - Phase-2 sweep rewritten to walk every object and opportunistically gzuncompress raw streams — no longer replicates the production method's filter/BT skip heuristic, so a regression that tightens those skips can't pass vacuously. - Loop wrapped in push_state/try/catch so partial mid-loop failure rolls back via pop_state instead of leaving _pdf_objects in a half-mutated state. - poc-make-fixture.php: gzcompress level pinned to 6 (deterministic across zlib versions); explicit false-return check; missing trailing newline added. - poc-replace-text.php: now passes both a matching and an unmatched needle so unmatched_needles + replacements_per_needle are exercised end-to-end. Missing trailing newline added. - replace_text_in_document: switched to single-pass array-form str_replace + per-needle substr_count pre-pass — faster on multi-substitution batches over large streams. - poc-make-fixture: namespace declaration dropped (concerns paired with the trailing-marker minor). 🟢 Minors - PSR-12 trailing newline on both PoC scripts. - @return docblock now uses array-shape syntax for type-safe destructuring. - .gitignore: docs/.idea/ → /.idea/ (JetBrains creates the dir at project root, not under docs/). - Trailing-marker comments dropped from poc-make-fixture (sapp's other source files don't use them). Verification: examples/poc-replace-text.php exits 0 with the new mixed substitution set; diagnostic surface populates unmatched_needles correctly; Phase-2 exhaustive sweep finds 0 residual needles and 1 placeholder hit.
…code-cmap # Conflicts: # src/PDFDoc.php
Blockers: - Phantom Tj/Tf operator detection inside string literals — raw strpos matched `Tj` inside `(Show Tj inside this string)`. Rewrote findNextOperator as a string-state-aware tokenizer that skips `(...)` literals (with backslash-escape + nested-paren depth tracking) and `<...>` hex strings. - unmatched_needles already populated by the merged loop fix (carried over from PR #1 strict-review hardening). - replacements_per_needle double-counted on cid_split_mismatch. spliceOperand returns the unchanged input on splice failure; the loop now checks `if ($spliced === $newOperandBytes) break;` and only bumps the counter on actual mutation. - Tf back-scan window: preg_match scan now constrained to a 64-byte window ENDING at the Tf operator (\z anchor). Previously the search range went to end-of-stream and could match a far-ahead Tf. - Multi-occurrence regression: spliceOperand uses strpos (first occurrence only). Now wrapped in a `while strpos(...) !== false` loop with safety cap of 1024 iterations. - /Resources inheritance: collectPageFonts now walks `/Parent` chain up to 32 levels until a node with /Resources is found (PDF 1.7 §7.7.3.4). - Backslash-escape parity: findTjOperand now counts consecutive backslashes before each `(`/`)` and treats the paren as escaped iff the count is ODD. Previously `\\)` (2 backslashes + close) was mis-classified as escaped. - Encoding dict shape silent degradation: buildFontInfo detects inline `<< /BaseEncoding ... /Differences ... >>` dict shape, records it under new `encoding_dict_unhandled` diagnostic (per oid + font resource name), and falls back to /BaseEncoding for best-effort resolution. - Tf state across /Contents array entries: buildFontContext now detects pages whose /Contents is an array (>1 stream) and records them under `contents_array_pages` diagnostic so callers can detect the cross-stream-Tf-state-loss limitation. Concerns: - hex2bin '?:' anti-pattern (same as PR #1 `'0'` codepoint bug): findTjOperand hex shape now uses explicit `=== false` check. - bfrange DoS: parseBfrangeBlock now rejects ranges > 256 codes per Adobe Tech Note 5411 §1.4.5. - ext-mbstring requirement: composer.json now explicit. - Static encoding tables cached in FontEncoding::forName (per-name instance cache; tables are immutable so reuse is safe). - Identity-H partial resolution discards prefix: unknown CIDs now substitute U+FFFD instead of returning empty (keeps the resolved prefix available for needle matching, common in subset fonts). - PDF 1.7 §9.6.5.4 implicit-default-by-font-type documented in buildFontInfo PHPDoc; full implementation deferred to a follow-up. Minors: - DEL escape: escapePdfLiteral now `>= 0x80` (PDF spec allows 0x7F unescaped). - FQCN cleanup: FontEncoding/CMap referenced via short names (same namespace). - Triplicate Implementation Notes in docs/upstream-prs/06-tounicode- cmap/{proposal,design,tasks}.md replaced with canonical pointers to openspec/changes/feat-tounicode-cmap/. Phantom task ticks reverted: - 7.3 (Identity-H end-to-end round-trip) → unchecked, deferred to PR #10 (feat-text-replacement-api). - 7.5 (subset-font negative test) → unchecked, deferred to PR #10. - 4.3 (/Resources inheritance) kept checked — now actually shipped. Verification: all 7 gates green (poc-replace-text, poc-filter-chain-roundtrip, poc-filter-roundtrip-{asciihex, runlength, ascii85, lzw}, poc-tounicode-cmap).
| // already recorded inside spliceOperand. | ||
| break; | ||
| } | ||
| $newOperandBytes = $spliced; |
There was a problem hiding this comment.
🟡 Concern — Multi-occurrence loop silently caps when placeholder contains needle
The new while (strpos($resolvedText, $needle) !== false) wrapper (correct fix for the original Finding 5 multi-occurrence regression) silently caps at 1024 iterations if the placeholder itself contains the needle — e.g. substituting 'Jansen' → 'Jansen-redacted'. The loop re-finds the needle inside the placeholder bytes every iteration; the 1024 cap stops the runaway but emits no diagnostic.
Caller running replace_text_in_document(['Jansen' => 'Jansen-redacted']) gets replacements_per_needle['Jansen'] = 1024 with zero signal that the overlap exists.
Suggested fix: detect strpos($placeholder, $needle) !== false once before the loop and either (a) reject the substitution with a new placeholder_contains_needle diagnostic, or (b) splice only the matches present at loop entry (snapshot positions first, then mutate).
| // span more than 256 codes. Reject larger ranges to bound | ||
| // memory use against malicious CMaps that would expand to | ||
| // (e.g.) 65K entries via `<0001> <FFFF> <0041>`. | ||
| if (($hiInt - $loInt) > 255) { |
There was a problem hiding this comment.
🟢 Minor — bfrange 256-cap silently drops valid-but-non-compliant large ranges
The if (($hiInt - $loInt) > 255) continue; cap (good fix for the original Finding 15 DoS) is correct per Adobe Tech Note 5411 §1.4.5 — but real-world non-compliant CMaps emitted by some PDF generators do span larger ranges. The current code silently continues them, leaving every CID in that range unmapped. No diagnostic surface, so the caller has no way to detect 'we just refused to parse 60% of your CMap'.
Suggested fix: track skipped ranges in a private $rejectedLargeRanges counter on the CMap and expose via a getParseDiagnostics(): array method (or log via p_error). The cap stays; just surface the rejection.
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Re-review of 5bce5ea7 — APPROVE 🟢
Substantial good-faith engagement with the first-pass strict review. Verifying each prior finding against the local checkout:
Prior 🔴 Blockers (9): 8 resolved + 1 accepted as documented-limitation (Tf state across multi-stream /Contents — diagnostic-only fix via contents_array_pages, behaviour unchanged but surfaced to callers).
Prior 🟡 Concerns (17): 9 resolved — DoS bfrange cap, ext-mbstring in composer.json, static-table instance cache, U+FFFD prefix preservation, hex ?: anti-pattern audit, design.md / tasks.md cleanup, Adobe §1.4.5 (via cap), PDF §9.6.5.4 documented limitation, 52/52 task ticks reverted with deferral to PR #10. 8 left open as follow-up — see inline replies on the still-open threads.
Prior 🟢 Minors (4): 2 resolved (FQCN cleanup, DEL 0x7F unescaped). 2 remain (public constructor on FontEncoding, MacRoman table holes) — pre-existing style/coverage items.
Bonus: method_exists guards added defensively across the new dispatch paths.
New findings:
- 🟡 NEW-1 (discussion_r3318321019) — multi-occurrence loop silently caps at 1024 when placeholder contains needle
- 🟢 NEW-2 (discussion_r3318321530) — bfrange 256-cap silently drops valid-but-non-compliant large ranges (no diagnostic)
Verdict rationale: all 9 prior 🔴 blockers substantively addressed; remaining items are pre-existing 🟡/🟢 deferred for follow-up tracking (mostly diagnostic-surface additions and a perf refactor). 22 prior threads resolved, 10 left open as documented follow-up. Standard mode → APPROVE.
Quality of fixes (highlights):
findNextOperatoris now a proper string-state-aware tokeniser with nested-paren + backslash-escape handling — correct on all the edge cases I mentally walked- Backslash-parity logic in
findTjOperandcorrectly handles\\),\),\\\) - /Resources
/Parentchain walk capped at 32 depth - Encoding-dict diagnostic (
encoding_dict_unhandled) added in the same pattern as the newcontents_array_pages— consistent observability surface for documented limitations
Nice work.
feat(cmap): ToUnicode CMap + FontEncoding + text-space matching (SAPP PR #6)
2 participants
Summary
The heaviest single PR in the text-replacement series — and the one that unblocks production Woo PDFs. Refactors
replaceTextInDocumentfrom byte-space to text-space matching by introducing CMap parsing and per-page font resolution. Word-generated documents using Identity-H subset fonts (the >95% case) now work.Stacked on #7 feat/lzw-decode.
What landed (1646 +, 123 −)
New top-level classes
src/CMap.php(~240 LOC)beginbfchar+ bothbeginbfrangeshapes + multi-codepoint targets, NFC-normalisedsrc/FontEncoding.php(~170 LOC)/WinAnsiEncoding(full Table D.2),/MacRomanEncoding,/StandardEncoding,/Identity-H/VdiscriminatorsBoth are pure value-object types with no dependencies on
PDFDoc/PDFObject— upstream-friendly shape.PDFDoc refactor
replaceTextInDocumentnow:/Catalog/Pagesto build per-page font context (8 new helper methods)/ContentsreferenceTf, parsesTjoperands (both(literal)and<hex>shapes)font_encoding_misseswhen forward map can't encode (PR feat(cmap): ToUnicode CMap + text-space matching (PR #06, stacked on #7) #8 adds the Helvetica fallback)cid_split_mismatchwhen match would cross a multi-codepoint CID interior(literal)vs<hex>shapeSpec contract
Pinned by
openspec/changes/feat-tounicode-cmap/:font_encoding_misses+cid_split_mismatchBackwards compatibility
The existing
examples/poc-replace-text.phpcontinues to exit 0 with identical diagnostic output (streams_modified=1, residual_needles=0, placeholder_hits=1). WinAnsi's printable ASCII range maps byte → identical codepoint, so text-space matching lands at the same byte offsets as the prior byte-space matcher. The(literal)/<hex>shape preservation makes the round-trip byte-for-byte clean.PHP gotcha caught during testing
mb_convert_encoding(..., 'UTF-8', 'UTF-32BE')returns the STRING'0'for codepoint 48 (ASCII digit zero). My?:ternary then substituted''because'0'is falsy in PHP. Fixed with an explicit=== falsecheck in bothFontEncoding::byteToUnicodeandCMap::codepointToUnicode. Surfaced by the round-trip test's WinAnsi printable-ASCII loop. Worth surfacing in the eventual upstream PR's design notes.Test plan
php examples/poc-tounicode-cmap.php— 5 REQs, 0 assertion failures (CMap parse cases + FontEncoding round-trip + existing-PoC regression + forward-map encoding + unencodable-char null)php -lclean on all touched filesFiles changed (1646 +, 123 −)
src/CMap.php— NEWsrc/FontEncoding.php— NEWsrc/PDFDoc.php— major refactor ofreplaceTextInDocument+ 8 new helper methodsexamples/poc-tounicode-cmap.php— new verification gatedocs/upstream-prs/06-tounicode-cmap/{proposal,design,tasks}.md— implementation notesopenspec/changes/feat-tounicode-cmap/tasks.md— 52/52 completeDependencies + ordering
Out of scope (explicit)
font_encoding_missesrecords + skips; PR feat(cmap): ToUnicode CMap + text-space matching (PR #06, stacked on #7) #8 recovers viaq /F-fallback Tf ... Q)/Differencesarrays on simple-font/Encoding— rare in modern PDFs; not implemented