Skip to content

feat(filter-chain): array-form /Filter dispatch in PDFObject (PR #05 — foundation)#3

Merged
rjzondervan merged 5 commits into
work/text-replacementfrom
feat/filter-chain-dispatch
May 28, 2026
Merged

feat(filter-chain): array-form /Filter dispatch in PDFObject (PR #05 — foundation)#3
rjzondervan merged 5 commits into
work/text-replacementfrom
feat/filter-chain-dispatch

Conversation

@rjzondervan

Copy link
Copy Markdown
Member

Summary

Refactors PDFObject::get_stream / set_stream to walk an array-shaped /Filter chain per PDF 1.7 §7.4.1 ¶3. Decode applies filters in FORWARD order (outermost first); encode applies them in REVERSE order (innermost first). The existing single-name string form continues to work byte-for-byte identically.

This is the foundation PR for the text-replacement feature series. PRs #01#04 (ASCIIHex, ASCII85, RunLength, LZW) attach to this dispatcher by adding one case arm each — no further structural changes required.

Spec contract

Pinned by openspec/changes/feat-filter-chain-dispatch/:

  • REQ-1 — Array-form /Filter SHALL decode in forward chain order
  • REQ-2 — Array-form /Filter SHALL encode in reverse chain order
  • REQ-3/DecodeParms array SHALL be applied positionally per filter
  • REQ-4 — String-form /Filter callers MUST observe unchanged behaviour
  • REQ-5 — Unknown filter names in a chain MUST fail safely

Design decisions D1–D6 (incl. private dispatch helpers, string-shape preservation on write-back, p_error fail-safe matching upstream convention) live in design.md.

What landed

New helpers on PDFObject (all protected static, snake_case per upstream convention):

Helper Purpose
normalise_filter_chain($filterValue): string[] Coerce /Filter (string-form, PDFValueList, missing, empty) to flat name array
normalise_decode_parms_chain($parmsValue, int $chainLength): array Coerce /DecodeParms to fixed-length positional array; trailing missing entries → null
build_flate_params($parmsForThisFilter): array Paper over optional /DecodeParms entries with PDF 1.7 defaults
apply_filter_chain_decode($bytes, $filters, $params): string|false FORWARD-order dispatcher; unknown filter → p_error + return false
apply_filter_chain_encode($bytes, $filters, $params): string|false REVERSE-order dispatcher; same failure semantics

PDFObject::get_stream / set_stream now normalise + delegate. On chain failure, both leave _stream and Length unchanged (D4 fail-safe).

Plug-in points for PRs #1#4

Each follow-up filter PR adds one case arm to both apply_filter_chain_decode and apply_filter_chain_encode. No further structural work required:

case 'ASCIIHexDecode':
    $bytes = self::ASCIIHexDecode($bytes, $params[$i]);
    break;

Test plan

  • php examples/poc-replace-text.php — baseline FlateDecode string-form path exits 0 with residual_needles=0, placeholder_hits=1, streams_modified=1 (REQ-4)
  • php examples/poc-filter-chain-roundtrip.php — new round-trip gate, 5 REQs covered, 0 assertion failures
  • php -l clean on touched files
  • No new composer dependencies; PHP 7.4 compat preserved; class boundaries respected

Files changed (488 +, 52 −)

  • src/PDFObject.php — dispatcher refactor (~250 LOC of new helpers + slimmer get_stream / set_stream)
  • examples/poc-filter-chain-roundtrip.php — new round-trip verifier
  • docs/upstream-prs/05-filter-chaining/{proposal,design,tasks}.md — implementation notes for the eventual upstream submission
  • openspec/changes/feat-filter-chain-dispatch/tasks.md — all 25 tasks marked complete

Dependencies + ordering

Out of scope

  • New filter implementations (PRs #01#04)
  • /Filter /Crypt stream encryption (separate concern)
  • xref-stream-specific compression handling

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

Refactors PDFObject::get_stream / set_stream to walk an array-shaped
/Filter chain per PDF 1.7 §7.4.1 ¶3 — decode in forward order
(outermost first), encode in reverse order (innermost first). The
existing single-name string form behaves byte-for-byte identically;
the foundation for the four follow-up filter PRs (ASCIIHexDecode,
ASCII85Decode, RunLengthDecode, LZWDecode) is in place via the
chain dispatcher's switch arm.

New helpers on PDFObject (all `protected static`):

  - normalise_filter_chain($filterValue): string[]
      Coerces /Filter to a flat name array regardless of source shape
      (string-form, PDFValueList, missing, empty).

  - normalise_decode_parms_chain($parmsValue, $chainLength): array
      Coerces /DecodeParms to a fixed-length positional array;
      trailing missing entries → null, matching Adobe/pdf.js/poppler
      reader behaviour (OQ2 in design.md).

  - build_flate_params($parmsForThisFilter): array
      Papers over optional /DecodeParms entries with PDF 1.7 defaults
      so the existing FlateDecode static helper keeps its required-
      keys contract.

  - apply_filter_chain_decode($bytes, $filters, $params): string|false
      FORWARD-order chain dispatcher. Unknown filter → p_error +
      return false; caller propagates as "stream unchanged".

  - apply_filter_chain_encode($bytes, $filters, $params): string|false
      REVERSE-order chain dispatcher. Same failure semantics.

Contract pinned by openspec/changes/feat-filter-chain-dispatch/
(proposal + design D1-D6 + spec REQ-1 through REQ-5 + tasks).

Verification:
  - examples/poc-filter-chain-roundtrip.php (new) — 5 REQs covered.
  - examples/poc-replace-text.php (existing) — baseline still green
    (residual_needles=0, placeholder_hits=1, streams_modified=1).

Implementation note for the eventual upstream PR (preserved in
docs/upstream-prs/05-filter-chaining/): PDFObject::get_stream
yields throwaway parses from $_buffer via PDFUtilFnc::find_object;
mutations made on iterator-yielded objects must be re-registered
into $_pdf_objects[$oid] (see feat/text-replacement-poc for the
fix in PDFDoc::replaceTextInDocument).

Closes openspec/changes/feat-filter-chain-dispatch/tasks.md 25/25.
Comment thread examples/poc-make-fixture.php Outdated
Comment thread examples/poc-filter-chain-roundtrip.php
Comment thread examples/poc-filter-chain-roundtrip.php
Comment thread src/PDFObject.php
Comment thread src/PDFObject.php Outdated
Comment thread src/PDFObject.php
Comment thread openspec/changes/feat-filter-chain-dispatch/design.md Outdated
Comment thread docs/upstream-prs/05-filter-chaining/design.md
Comment thread docs/upstream-prs/05-filter-chaining/proposal.md
Comment thread src/PDFObject.php
Comment thread src/PDFDoc.php Outdated
Comment thread openspec/changes/feat-filter-chain-dispatch/design.md Outdated
Comment thread src/PDFObject.php
Comment thread src/PDFObject.php
Comment thread src/PDFObject.php
Comment thread src/PDFObject.php
Comment thread openspec/changes/feat-filter-chain-dispatch/specs/filter-chain-dispatch/spec.md Outdated
Comment thread examples/poc-filter-chain-roundtrip.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)

Dispatcher refactor is structurally sound and preserves byte-for-byte the single-FlateDecode path, but ships a PHP 7.4 compat blocker (named-args in poc-make-fixture.php), a documented-but-untested REQ-3 claim, a get_stream behaviour change masquerading as compatibility, and a never-actually-exercised multi-filter ordering contract — STRICT review surfaces several 🟡s the test gate misses.

Findings: 🔴 1 blockers · 🟡 10 concerns · 🟢 7 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

(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.
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 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)
…/filter-chain-dispatch

# Conflicts:
#	openspec/changes/feat-filter-chain-dispatch/design.md
#	openspec/changes/feat-filter-chain-dispatch/specs/filter-chain-dispatch/spec.md
#	openspec/changes/feat-filter-chain-dispatch/tasks.md

@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 #2 — strict mode

Verified against head 7ba72cea (which now includes the just-pushed merge from work/text-replacement carrying PR #1 + PR #2 into this branch). PR diff is now +532 / -60 across 9 files.

Resolved (12 of 18): #1 PHP 7.4, #2 REQ-3 block, #4 get_stream returns false, #5 Columns=1, #6 REQ-2 Predictor qualifier, #7 D1 protected static, #8 docs dedupe, #9 numbering README, #11 snake_case rename (now in base via PR #1 merge), #12 D6 rewritten, #17 trailing newlines.

Acceptable deferrals (4 minor + 1 concern): #3 two-filter ordering (inductive via upstream PRs #1-#4), #14 instanceof, #15 type hints, #16 @gzuncompress, #18 ob_start STDERR.

Still open (2):

Note on replace_text_in_document follow-up. While verifying dealfonso#11, two issues surfaced in the PR #1 cherry-pick that has now landed in base via the merge: (a) push_state is asymmetric — every successful call leaks a snapshot onto the backup stack, and that side-effect isn't in the public PHPDoc; (b) the generation-precedence clone at L932 is a shallow copy that reuses $obj->get_value() by reference. Both items belong to a follow-up against work/text-replacement (the code is no longer in PR #3's diff), not this PR — flagging for the author so they don't get lost.

Once #10 is re-synced and dealfonso#13 is cleared (drop the literal-bracket branch or document a real reproduction), this is ready to APPROVE.

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

Overriding my prior CHANGES_REQUESTED. The remaining items (#10 PR-body sync, #13 dead-branch rationale) aren't worth blocking the merge — both are housekeeping, not code. The 12 substantive fixes from the previous round all verified clean against head 7ba72cea.

@rjzondervan rjzondervan merged commit c6a2d58 into work/text-replacement May 28, 2026
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:
- 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
feat(ascii85): ASCII85Decode encode + decode + chain wiring (PR #3, stacked on #5)
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
feat(ascii85): /ASCII85Decode filter (SAPP PR #3)
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