Skip to content

PoC: text replacement on FlateDecode content streams#1

Merged
rjzondervan merged 2 commits into
work/text-replacementfrom
feat/text-replacement-poc
May 28, 2026
Merged

PoC: text replacement on FlateDecode content streams#1
rjzondervan merged 2 commits into
work/text-replacementfrom
feat/text-replacement-poc

Conversation

@rjzondervan

Copy link
Copy Markdown
Member

Summary

Proof-of-concept implementation of PDFDoc::replaceTextInDocument(array $substitutions): array — the foundation for the text-replacement feature being staged on work/text-replacement for eventual upstream contribution back to dealfonso/sapp.

Verified end-to-end on a synthesised WinAnsi / FlateDecode / single-Tj fixture: the round-tripped output PDF contains zero residual needles and at least one placeholder hit. Diagnostic surface returns streams_modified=1, replacements_per_needle={'Jan Jansen': 1}, unmatched_needles=[].

Scope (PoC — locked in the consumer change pdf-anonymisation design §D4)

The follow-up work for real-world Woo PDFs (Word-generated Identity-H subset fonts + TJ kerning) is the next step on this branch — this PR locks the foundation.

Implementation note

PDFDoc::get_object_iterator() yields PDFObject instances parsed fresh from $_buffer via PDFUtilFnc::find_object. Mutations made on those instances live only on the iterator's throwaway references — they're invisible to to_pdf_file_b's rebuild loop (which calls get_object(oid)_pdf_objects[oid] fallback → fresh parse from the original $_buffer). The PoC registers the mutated object back into $this->_pdf_objects[$oid] inside the replace loop. This is a finding that will need to be documented in the eventual upstream PR.

Test plan

  • php examples/poc-replace-text.php exits 0 locally
  • Verify output PDF round-trips through PDFDoc::from_string without errors
  • Re-extract sweep finds 0 residual "Jan Jansen" and ≥1 "[PERSON: 7]"
  • Reviewer opens examples/poc-output.pdf in a PDF reader and confirms the placeholder renders

Files

  • src/PDFDoc.php — new replaceTextInDocument() method (~100 LOC including PHPDoc)
  • examples/poc-make-fixture.php — fixture generator
  • examples/poc-fixture.pdf — checked-in fixture for repeatable verification
  • examples/poc-replace-text.php — validation gate, asserts the round-trip cleanliness
  • .gitignore — ignore docs/.idea/ (IDE scratch) and examples/poc-output.pdf (test artifact)

…ecode content streams

Adds PDFDoc::replaceTextInDocument(array $substitutions): array — the
PoC entry point for the work/text-replacement integration branch.
Scope per the consumer OpenRegister change pdf-anonymisation §D4:

  - FlateDecode-encoded streams only (upstream PRs #1-#5 will add
    the other filters + chain dispatch).
  - Byte-level literal match on the decoded stream (upstream PR #6
    adds Identity-H / ToUnicode CMap font resolution).
  - No TJ kerning-array flattening — works on streams whose needle
    lives inside a single Tj operator (upstream PR #7 adds TJ
    flattening).
  - No font switch — placeholder renders in the font active at the
    match position (upstream PR #8 adds the subset-font fallback).

Companion artefacts:

  - examples/poc-make-fixture.php — synthesises a minimal WinAnsi /
    FlateDecode / single-Tj fixture that satisfies the PoC scope.
  - examples/poc-fixture.pdf — checked-in output of the generator
    so the verify script is repeatable without a regeneration step.
  - examples/poc-replace-text.php — decode -> replace -> re-encode ->
    re-extract validation gate. Asserts the round-tripped PDF has
    zero residual needles and at least one placeholder hit, per the
    consumer-side design D5.

Implementation note that will need to land in the eventual upstream
PR (PRs #6/#8): PDFDoc::get_object_iterator yields PDFObject
instances parsed fresh from $_buffer via PDFUtilFnc::find_object.
Mutations made on those instances live only on the iterator's
throwaway references — they're invisible to to_pdf_file_b's rebuild
loop (which calls get_object(oid) -> _pdf_objects[oid] fallback ->
fresh parse from the original $_buffer). The fix is to register the
mutated object into $this->_pdf_objects[$oid] inside the replace
loop so subsequent get_object() calls return our copy. Worth a
dedicated note in the upstream PR write-up alongside the new API.

Verified: php examples/poc-replace-text.php exits 0 with
streams_modified=1, residual_needles=0, placeholder_hits=1.
Comment thread examples/poc-make-fixture.php Outdated
Comment thread src/PDFDoc.php Outdated
Comment thread src/PDFDoc.php Outdated
Comment thread src/PDFDoc.php Outdated
Comment thread src/PDFDoc.php Outdated
Comment thread src/PDFDoc.php Outdated
Comment thread src/PDFDoc.php Outdated
Comment thread src/PDFDoc.php Outdated
Comment thread examples/poc-replace-text.php
Comment thread src/PDFDoc.php Outdated
Comment thread examples/poc-make-fixture.php Outdated
Comment thread examples/poc-make-fixture.php
Comment thread examples/poc-replace-text.php
Comment thread examples/poc-make-fixture.php Outdated
Comment thread examples/poc-make-fixture.php
Comment thread examples/poc-make-fixture.php Outdated
Comment thread src/PDFDoc.php Outdated
Comment thread src/PDFDoc.php Outdated
Comment thread src/PDFDoc.php
Comment thread .gitignore Outdated
Comment thread examples/poc-make-fixture.php

@WilcoLouwerse WilcoLouwerse left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strict-mode review (REQUEST_CHANGES)

PoC functionally works for the locked one-Tj-FlateDecode fixture and the write-back hack is sound for to_pdf_file_s(true), but the new method has several latent correctness bugs (camelCase in a snake_case codebase, /Fl abbreviation + chained-filter blind spot, unescaped PDF-literal injection, misleading streams_scanned counter, silent breakage of to_pdf_file_s(false) and of the empty-_pdf_objects fast path) and the fixture generator requires PHP 8.0+ named arguments while the project targets PHP >=7.4.

Findings: 🔴 6 blockers · 🟡 11 concerns · 🟢 4 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

  • examples/poc-make-fixture.php:69 — Fixture generator uses PHP 8.0+ named arguments — breaks PHP 7.4 target
  • src/PDFDoc.php:814 — Method name violates the file's snake_case public-API convention
  • src/PDFDoc.php:845 — Placeholder bytes are not PDF-string-escaped — (, ), \ in a replacement corrupt the stream
  • src/PDFDoc.php:837streams_scanned undercounts — gated by BT/ET heuristic, contradicting its docstring
  • src/PDFDoc.php:864 — Bypasses add_object — clobbers prior mutations and skips generation bookkeeping
  • src/PDFDoc.php:864 — Write-back silently breaks to_pdf_file_b(false) — the empty _pdf_objects fast path is destroyed

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

@WilcoLouwerse WilcoLouwerse left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — four new findings introduced by the fix commit. Verdict to follow.

Comment thread src/PDFDoc.php
Comment thread src/PDFDoc.php
Comment thread src/PDFDoc.php
Comment thread src/PDFDoc.php

@WilcoLouwerse WilcoLouwerse left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ APPROVE — re-review (Standard mode)

All 21 prior findings substantively addressed across 1c98aa0. PoC scope holds; ready to proceed on the staging branch.

Resolved from prior review (21)

  • 🔴 Blockers (6/6): PHP 7.4 named-args compat ✅ · replace_text_in_document snake_case ✅ · PDF-string escape contract documented ✅ · streams_scanned semantics fixed (+ new content_streams_scanned) ✅ · add_object precedence preserved ✅ · to_pdf_file_b(true) mandate documented ✅
  • 🟡 Concerns (11/11): all addressed; stream-EOL convention is validated implicitly by the sapp round-trip (smalot/pdfparser cross-check was suggested but not run — acceptable for the PoC).
  • 🟢 Minors (4/4): all fixed.

(I lack resolveReviewThread permissions on this repo, so the 21 prior threads remain technically "unresolved" on GitHub — base work/text-replacement is unprotected so this does not block merge. Listing the verification status above for the audit trail.)

New findings — informational, not blocking

None of the new findings block the PoC's documented scope. Worth folding into the upstream PR series (#6/#8/#10) where the runtime parameter validation and rollback semantics will need to be tightened anyway.

rjzondervan added a commit that referenced this pull request May 28, 2026
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.
rjzondervan added a commit that referenced this pull request May 28, 2026
Adds two new commits on top of the existing PR #3 branch:

  1. The PR #1 fix cherry-picked: PoC's PDFDoc::replaceTextInDocument
     rename to snake_case + add_object write-back + escape-contract
     docblock + streams_scanned semantics + Phase-2 exhaustive sweep
     in the verify gate + namespace/named-arg fixes in
     poc-make-fixture.php. Same content as the corresponding PR #1
     fix; arrives here because PR #3 has a cherry-pick of the PoC
     and the PR #1 review's blockers apply to that copy too.

  2. This commit: PR #3-specific fixes from Wilco's first-pass
     strict review.

🔴 PR #3 Blocker
  - PHP 7.4 named-args in poc-make-fixture.php — addressed by
    commit (1) above (the cherry-picked PR #1 fix replaces named
    args with positional and pins gzcompress level to 6).

🟡 PR #3 Concerns
  - get_stream(false) on chain failure now returns `false` (matching
    upstream's pre-refactor `return p_error(...)` semantics) rather
    than the raw $this->_stream. Callers using the `if ($decoded ===
    false) continue;` idiom get the skip behaviour they did before
    the dispatcher refactor. spec.md REQ-5 scenario updated to match.
  - build_flate_params /Columns default fixed from 0 → 1 per
    PDF 1.7 §7.4.4.3 Table 8. Hidden today because Predictor=1
    short-circuits; would have bitten the moment a PNG-predicted
    stream landed without explicit /Columns.
  - REQ-2 (encode-decode round-trip lossless) spec scenario
    tightened to "Predictor absent or = 1" — round-trip with a
    non-trivial PNG predictor is acknowledged as out of scope (no
    encode-side predictor support); future `feat-flate-predictor-encode`
    closes the gap.
  - design.md D1 wording fixed from "We add two `private` helpers"
    to "We add two `protected static` helpers", matching what
    shipped. Visibility choice justified (test-suite subclasses can
    stub for chain-ordering assertions; matches FlateDecode).
  - design.md D6 rewritten to reflect what the gate actually
    exercises (5 contract scenarios including the new REQ-3 smoke
    test) instead of the original "synthetic two-filter PDF"
    promise. Two-filter ordering is locked at spec layer but
    proved inductively by upstream-PRs #1-#4 each pairing the new
    codec with /FlateDecode.
  - examples/poc-filter-chain-roundtrip.php gained a REQ-3 block
    smoke-testing /DecodeParms positional routing — single-filter
    chain with explicit `/DecodeParms [<</Predictor 1 /Columns 4>>]`
    that round-trips cleanly.
  - docs/upstream-prs/05-filter-chaining/{proposal,design,tasks}.md
    no longer carry the duplicated 18-line "Implementation note"
    block. Each file now links to
    `openspec/changes/feat-filter-chain-dispatch/design.md` as the
    canonical artefact — single source of truth.
  - docs/upstream-prs/README.md now explicitly explains the two
    numbering schemes: directory `01..08` is the upstream-submission
    order; fork PR numbers are the local landing order. Foundation
    (`05`) lands first locally so dependent codecs (`01..04`) can
    attach. Cross-references use the `upstream-PR #NN` form.
  - replaceTextInDocument camelCase fixed — via the PR #1 fix
    cherry-pick (commit 1 above) which renames to snake_case
    replace_text_in_document.

🟢 Minors deferred
  - normalise_filter_chain dead defensive branch — the branch
    handles a `null` input that may or may not be reachable
    depending on PDFObject construction path; leaving as-is.

Verification: both poc-replace-text.php (PR #1 gate) and
poc-filter-chain-roundtrip.php (this PR's gate, now with the REQ-3
smoke test block) exit 0 after these two commits.

NOTE on the stacked-PR / no-force-push approach: this PR's commit
diff still references the OLD scaffold content from PR #2 (the
pre-rewrite spec.md / proposal.md / etc.). PR #2's mechanical
pass (REQ-NNN numbering + GIVEN clauses + Status/Purpose stub
headers) is shipped on `chore/openspec-scaffold` and will arrive
here naturally when this branch is rebased onto an updated
`work/text-replacement` after PR #2 merges. The spec amendments in
this commit will then merge with PR #2's rewrites — small expected
conflicts in spec.md's REQ-5 + REQ-2 scenarios, resolvable inline.
@rjzondervan rjzondervan merged commit 2129a12 into work/text-replacement May 28, 2026
rjzondervan added a commit that referenced this pull request May 28, 2026
Adds a merge commit bringing `feat/filter-chain-dispatch`'s fix
commits (PoC snake_case rename + chain dispatcher get_stream-returns-
false + Columns default fix + docs/upstream-prs deduplication) into
this branch, and a follow-up commit with PR #4 specific fixes.

🔴 PR #4 Blockers
  - ASCIIHexDecode failure-path return changed from `$_stream` (raw
    input) to `false`. Matches the chain dispatcher's `=== false`
    short-circuit contract and upstream `p_error`'s default return.
    Critical correctness fix: prior to this, a malformed outer-
    ASCIIHex layer in a `[/ASCIIHexDecode /FlateDecode]` chain
    silently fed the unmodified illegal bytes to the inner
    FlateDecode arm — silent data corruption.
  - PHPDoc on ASCIIHexDecode updated: `@return string|false` is now
    accurate (the failure paths return `false` per the previous
    fix). Description clarified to surface the chain-dispatcher
    contract requirement.
  - Negative chain test added in `poc-filter-roundtrip-asciihex.php`
    asserting the dispatcher short-circuits on outer-ASCIIHex
    failure: `[/ASCIIHexDecode /FlateDecode]` chain with malformed
    hex bytes MUST return `false` from `get_stream(false)`. This
    locks the failure-propagation contract that the previous bug
    silently bypassed.

🟡 PR #4 Concerns
  - 80-col line wrap edge case in `ASCIIHexEncode`: when the hex
    representation is a multiple of 80, the encoder previously
    appended `>` directly after the final chunk, producing an 81-
    char line. Now inserts a newline before `>` so no line exceeds
    80 chars. Locked by a new 40-byte (80-hex-char) test case.
  - `tasks.md` 1.2 corrected: shows the actual stacked-PR base
    (`feat/filter-chain-dispatch`) instead of `work/text-replacement`.
  - `tasks.md` 2.2 corrected: the failure-path returns `false` not
    raw input (matches the post-fix code).
  - `tasks.md` 4.1 corrected: case label is bare `'ASCIIHexDecode'`
    not `'/ASCIIHexDecode'` (the dispatcher strips the leading slash
    before the switch).
  - `docs/upstream-prs/01-asciihex-decode/{proposal,design,tasks}.md`
    no longer carry the duplicated 9-line implementation note.
    Each links to `openspec/changes/feat-asciihex-decode/design.md`
    as the canonical artefact (same pointer-only pattern as PR #3).

🟡 Concerns deferred to follow-ups
  - Numbering scheme reconciliation: covered by the
    `docs/upstream-prs/README.md` explainer in PR #3's fix commit
    (`upstream-PR #NN` vs fork PR # numbering), which arrives here
    via the merge.
  - Encode-dispatcher symmetry on infallible filters: ASCIIHexEncode
    documented as infallible; an explicit `=== false` symmetry would
    require a return-type contract change. Tabled.

🟢 Minors addressed in tests
  - Missing-EOD tolerance now has an assertion (`ASCIIHexDecode('48656C6C6F')
    === 'Hello'`).
  - Empty-input edge cases asserted: `'>'`, ` ' \n \t >'`.
  - Round-trip of empty input asserted: `decode(encode('')) === ''`.
  - Unnecessary `@` operator on `hex2bin` dropped (the alphabet
    validation above guarantees it can't legitimately fail).

🟢 Minors deferred
  - PHP case-insensitivity nit on `ASCIIHexDecode` collision —
    no action; PascalCase-on-filter-names is the established
    convention.
  - PHP 7.4 union return type in docs — the docs use PHPDoc-only
    notation already; the docs/upstream-prs note is reasonable.
  - ReflectionClass coupling — by design; documented.
  - `p_error` stdout assumption — acceptable; tests don't depend
    on the error channel routing.

Verification: all 3 gates exit 0
  - poc-replace-text.php (PR #1 gate)
  - poc-filter-chain-roundtrip.php (PR #3 gate)
  - poc-filter-roundtrip-asciihex.php (this PR's gate, with the
    new chain-failure-propagation test + 40-byte 80-col edge + the
    missing-EOD/empty-edge assertions)
rjzondervan added a commit that referenced this pull request May 28, 2026
feat(asciihex): ASCIIHexDecode encode + decode + chain wiring (PR #1, stacked on #3)
rjzondervan added a commit that referenced this pull request May 28, 2026
Blockers:
- ASCII85Decode silently accepted 1-char trailing partial groups,
  emitting 0 bytes via `$emit = $groupLen - 1` when $groupLen=1.
  PDF 1.7 §7.4.3 partial-group rule requires `2 ≤ k ≤ 4`. Now
  rejects with p_error + `return false`.
- REQ-1 test fixtures used `87cURD~>` claiming it encodes "Hell"
  — actual canonical encoding is `87cUR~>` (5 chars). The `D` was
  a stray 1-char partial that the spec-violation above accepted as
  zero bytes. Test now uses verified canonical encodings.
- PHPDoc + design + proposal + tasks all claimed `uuuuu` = exactly
  2^32-1. Arithmetic shows `uuuuu` = 84*(85^4+85^3+85^2+85+1) =
  4,437,053,124, which is 142,085,829 OVER the 2^32-1 cap. The
  spec-imposed max 5-char group is actually `s8W-!`. Comments and
  docs now state this correctly; overflow guard text rewritten.

Concerns:
- Same dead-code pattern as ASCIIHex/RunLength: all decoder failure
  paths now `return false` (not `$_stream`), matching dispatcher
  contract.
- Error path was returning the post-`<~`-strip mutated stream as
  "raw input" — moot now that all failure paths return false.
- Defensive 32-bit overflow code in encoder (`& 0xFFFFFFFF` + signed
  promote) was dead on 64-bit PHP and broken on 32-bit (literal
  `0xFFFFFFFF` parses as float on 32-bit, bitwise-AND coerces and
  breaks the math). Deleted with comment that 64-bit ints are
  required (upstream composer constraint already guarantees this).
- preg_replace whitespace-strip now null-guarded (PCRE compile
  failure / limit → p_error + return false, no PHP 8.1+ deprecation
  on strlen(null)).
- New negative tests: `~` mid-stream, `z` mid-group, 1-char partial
  group, overflow guard fires on `tttt~>` (yields 4,384,231,064 >
  2^32-1).
- Boundary coverage extended to n=1..9 (previously skipped
  multiples of 4 — exactly the boundary between full-group and
  partial-group code paths).
- New empty-payload edge cases: `<~~>` and bare `~>`.
- New chain-failure-propagation test (outer ASCII85 illegal char
  MUST short-circuit before inner FlateDecode runs).

Minors:
- docs/upstream-prs/03-ascii85-decode/{proposal,design,tasks}.md:
  replaced 16-line copy-pasted "Implementation note" blocks with
  pointers to canonical openspec/changes/feat-ascii85-decode/
  (same pattern as PRs #1/#2/#5).
- spec.md REQ-001 rewritten with verified canonical byte literals
  (Hell, Hello, Hello world!); REQ-004 expanded to cover all 6
  spec-violation paths.

Verification: all 5 gates green (poc-replace-text,
poc-filter-chain-roundtrip, poc-filter-roundtrip-asciihex,
poc-filter-roundtrip-runlength, poc-filter-roundtrip-ascii85).
rjzondervan added a commit that referenced this pull request May 28, 2026
Blockers:
- All 4 LZWDecode error paths returned $_stream (the encoded bytes)
  while the docblock claimed @return string|false. The chain
  dispatcher's `=== false` check was dead code; truncated bit
  stream, dict overflow, KwKwK overflow, and out-of-range code all
  silently leaked encoded LZW bytes through the rest of the chain.
  All four now `return false` per the dispatcher contract.
- applyPngPredictor's docblock said `@return string|null` but the
  method only returned `string|false` (via `return p_error(...)`).
  LZWDecode guarded with `if ($predicted === null)` — always false,
  so `$out = $predicted` assigned `false` (a bool) to a string-typed
  output. Fixed the docblock + LZWDecode now checks `=== false`.

Concerns:
- Same dead-code dispatcher pattern as other filters: addressed by
  the blocker fix above.
- EarlyChange/decoder-lag comment rewritten: the `+1` in
  `($nextCode + 1) >= $threshold` is the decoder-lag correction
  (orthogonal to EarlyChange — both EarlyChange=0 and =1 work
  with it). Previous comment conflated the two.
- Pre-existing PNG Sub-filter bug (`$data[$i] = ($data[$i] +
  $data[$i-1]) % 256` doing string-arithmetic on single-char
  strings) now reached through LZW too. Fixed to use the same
  ord()/chr() wrapping the Up filter already had.
- applyPngPredictor docblock now states the documented carried-over
  limitations (Predictor=2 silently treated as <10, Colors=1 only,
  BitsPerComponent=8 only, Paeth/Average filter bytes rejected) so
  reviewers know LZW+Predictor=15 (auto-select) image streams may
  silently fail.
- Asymmetric encode-path/predictor for `set_stream(false)` with
  /Predictor=12 is a pre-existing chain-dispatcher concern,
  documented as out-of-scope for this PR.

Minors:
- New tests: truncated bit stream (single 0x80 byte), out-of-range
  code (300 with empty dict), predictor rejection (Predictor=15 +
  Colors=3 triggers applyPngPredictor → false propagation), Sub
  filter byte coverage (exercises the previously-broken string-
  arithmetic loop), chain-failure propagation (outer truncated
  LZW MUST short-circuit before inner Flate runs).
- docs/upstream-prs/04-lzw-decode/{proposal,design,tasks}.md: 34-line
  copy-pasted "Implementation note" blocks replaced with pointers to
  canonical openspec/changes/feat-lzw-decode/ (same pattern as PRs
  #1/#2/#3/#5).

Verification: all 6 gates green (poc-replace-text,
poc-filter-chain-roundtrip, poc-filter-roundtrip-{asciihex,
runlength, ascii85, lzw}).
rjzondervan added a commit that referenced this pull request May 28, 2026
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).
rjzondervan added a commit that referenced this pull request May 28, 2026
Blockers:
- Only the first match per TJ operator was processed. processTjArray
  returned on the first hit; outer caller advanced past the entire
  TJ. Multi-needle TJs and same-needle-twice-in-one-TJ
  under-counted replacements_per_needle. Rewrote with an outer
  while-loop that re-resolves fragment texts after each splice and
  continues until no remaining needle matches. New REQ-006 in
  spec.md anchors the contract with two-needle and same-needle-
  twice scenarios.
- Missing isset check on $stats['replacements_per_needle'][$needle]++
  in processTjArray. Lazy-init kept as a belt-and-braces guard
  (the entry point already pre-keys via array_fill_keys, but the
  guard survives future entry-point refactors).
- Odd-length hex silently dropped a char (@hex2bin($hex) ?: '').
  PDF 1.7 §7.3.4.3 mandates implicit trailing-zero padding. Now
  pads via `if (strlen($hex) & 1) $hex .= '0'`. New REQ-005
  scenario covers this.
- Non-spec leniency: numeric tokenizer accepted [-+\d.eE]. PDF 1.7
  §7.3.3 forbids exponent notation in Numeric Objects. Tightened
  to [-+.0-9] so a producer's `e` glyph after a hex CID can't be
  swallowed as part of a number. New REQ-005 scenario covers this.
- Phantom-ticked tasks 5.4 and 5.5 (no fixtures). Reverted to
  unchecked with explicit deferral notes.

Concerns:
- Dead $matchedOnce assignment removed via the rewrite.
- Comments not treated as whitespace inside TJ arrays. Now skipped
  per PDF 1.7 §7.2.4. REQ-005 scenario.
- tj_arrays_modified diagnostic key now pre-initialised at top of
  replace_text_in_document (contract no longer fragile).
- preg_match-per-byte for whitespace replaced with strpos against
  the 6-byte PDF whitespace set (~50× faster on large TJ arrays).
- Shape inheritance: placeholder now always emitted as literal
  regardless of first matched fragment's shape (matches D2
  documented contract).
- `@hex2bin` defensive `@` removed — the odd-length check above
  makes the warning unreachable; PHP errors for other malformed
  shapes now surface to callers explicitly.
- end() / reset() pointer-advancing avoided in the new render path
  (group-based emission uses numeric indexing).
- Triplicate Implementation Note blocks in docs/upstream-prs/07-tj-
  flattening/{proposal,design,tasks}.md replaced with canonical
  pointers (same pattern as PRs #1/#2/#3/#4/#5/#6).
- Tf set inside q/Q leaks documented as a known gap (q/Q stack
  not maintained); deferred to a follow-up.

Verification: all 7 gates green (poc-replace-text,
poc-tounicode-cmap, poc-tj-flattening, plus the 4 filter PoCs).

Also fixed in this commit: poc-tj-flattening.php was calling
$doc->replaceTextInDocument() (camelCase) which doesn't exist after
PR #1's snake_case rename. Updated to replace_text_in_document.
rjzondervan added a commit that referenced this pull request May 28, 2026
Blockers:
- PHPDoc @return out of sync with frozen 12-key API. Was: 6 keys.
  Now: full @phpstan-type ReplaceTextStats with all 12 keys
  (streams_scanned, content_streams_scanned, streams_modified,
  replacements_per_needle, unmatched_needles, font_encoding_misses,
  cid_split_mismatch, encoding_dict_unhandled, contents_array_pages,
  tj_arrays_modified, subset_font_fallbacks_used,
  rejected_substitutions). Each key gets a one-line description.
- Silent broken-PDF emission in injectFallbackFontResource: when
  /Resources or /Resources/Font was an unresolvable indirect ref,
  the code fell through and emitted content referencing a font
  resource that was never added. Both paths now return null cleanly
  if the indirect resolution fails — no half-modified PDF.
- Collision-detection bugs: get_keys() on PDFValueReference could
  return false (TypeError on PHP 8.x in in_array); and existing
  keys stored with leading slash would miss the collision check.
  Both fixed: normalise rawKeys to array, strip leading `/` to
  canonical form before in_array.
- Phantom-ticked tasks 6.1/6.3/6.4/6.7 (subset-font fixture, real-
  reader spot-check) unticked with explicit DEFERRED notes
  explaining why each one isn't actually exercised.

Concerns:
- Triplicate 57-line "Implementation note" blocks in docs/upstream-
  prs/08/{proposal,design,tasks}.md replaced with canonical pointer
  to openspec/changes/feat-text-replacement-api/.
- Per-page injection cache properties (was $fallbackFontInjected /
  $fallbackFontOid, camelCase between method bodies) moved to the
  top of the class as $_fallback_font_injected / $_fallback_font_oid
  (PSR-12 §4.2 + snake_case discipline). PHPDoc on the properties
  documents the lifetime invariant (cached for instance lifetime;
  two consecutive replace_text_in_document calls reuse the same
  Helvetica font object).
- Renamed page-object re-registration in injectFallbackFontResource
  from direct $_pdf_objects[$oid] = $obj write to $this->add_object()
  call so generation-precedence + _max_oid bookkeeping match the
  content-stream write path.

Minors:
- @phpstan-type ReplaceTextStats annotation added to lock the
  contract in static analysis tooling.
- Diagnostic-key ordering: code now matches the canonical
  alphabetical-by-category order documented in design.md.

Also fixed in the merge:
- examples/poc-text-replacement-api.php was calling camelCase
  $doc->replaceTextInDocument() which doesn't exist after PR #1's
  snake_case rename. Updated to replace_text_in_document.

Verification: all 8 gates green (poc-replace-text,
poc-filter-chain-roundtrip, poc-filter-roundtrip-{asciihex,
runlength, ascii85, lzw}, poc-tounicode-cmap, poc-tj-flattening,
poc-text-replacement-api).
rjzondervan added a commit that referenced this pull request May 28, 2026
feat(asciihex): /ASCIIHexDecode filter (SAPP PR #1)
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.

2 participants