Correct the README for 4.0, and give each bridge its own page - #38
Merged
Conversation
jakejackson1
force-pushed
the
docs/readme-4.0-accuracy
branch
4 times, most recently
from
August 24, 2026 02:38
178ef8d to
41b4295
Compare
Documentation only. No behaviour changes, no code changes.
Eight claims in the README no longer matched src/, each checked rather than
taken on faith:
- "Wording is API" contradicted the four other places saying codes are
stable and wording is not, and its second half described one outcome
twice: turning up untranslated and reverting to English are the same
thing. Rewording retires the msgid, which msgmerge reports as obsolete.
- sanitizeForDisplay()'s steps were listed with the trim before the UTF-8
repair. It runs after: "abc \xC3" returns "abc", not "abc ".
- isset($file[0]) was the wrong warning for a multi-file field. A failed
transfer leaves no hole, it renumbers: with entries 0 and 2 failing,
count() is 1 and $file[0] is the file the client selected second.
- ErrorCode listed UNKNOWN_TRANSFER_ERROR among "the eight UPLOAD_ERR_*
outcomes". There are seven; it is forUploadError()'s fallback, which
UPLOAD_ERR_OK also gets.
- The Interfaces table sent a custom FileInfoInterface at FileInfo's
`: FileInfo` setter signatures. The interface declares no return type on
the three setters, which is what made it implementable in 4.0.
- The custom-validation example called __() with no
`use function GravityPdf\Upload\__;`, the mistake the paragraph below it
warns about; under WordPress it reaches the global __().
- sanitizeNameWithExtension()'s signature omitted its $reserved parameter.
- $displayName was assigned and never used in the single-file example.
Added what a caller could not find: Filename's public predicates, which exist
so a custom storage backend can ask what the shipped one asks; __call()'s
BadMethodCallException; the ArrayAccess read and write semantics;
FileType::allow()'s empty-side guard; Size's parse failure; and that
ext-mbstring backs the getErrors() strings as well as the filenames.
The fork list was a 3.x list on a 4.0 README, naming getDirectory() and the
upstream import while omitting FileList, uploadValid() and translation.
Twelve bullets to seven.
A bridge off the $_FILES path now gets a page under docs/, and the README
holds only what every bridge shares. docs/psr7.md takes the PSR-7 bridge;
docs/base64-uploads.md does the same for a file posted as base64 in a JSON
body. Both checkers read TmpUploadFile and the tmp-file cleanup out of
README.md, so those two snippets exist once however many bridges ship.
Three more blocks moved, for readers the usage path does not serve:
docs/api-reference.md every public method, what it takes and throws
docs/extending.md ValidationInterface and StorageInterface
docs/translation/README.md the hook, the catalogue, the __() marker
Each leaves a stub. "Reacting to a failure rather than showing it" stays,
being about ErrorCode rather than translation. Three things could not move:
FileSystemTest reads the deny-list table out of README.md, and both bridge
checkers read TmpUploadFile and the cleanup from there. docs/ ships, so links
between the two halves stay relative and resolve from an installed vendor/.
Both bridge pages were then deslopped. Three facts in the base64 page were
stated twice, once in a snippet comment and again in the prose below it; its
post_max_size claim was contradicted by PHP's own 8M default, and the
whitespace margin it put a figure on assumed LF at 76 columns. The PSR-7
page's moveTo() section claimed the file is stored twice and then that the
second call finds nothing there; PSR-7 requires moveTo() to remove the
original and to raise on a second call, so only the second half was true.
CLAUDE.md and the CHANGELOG described the dist package as src/, composer.json
and four Markdown files, which predates docs/ and i18n/ shipping.
README 1023 -> 560 lines. No link across README.md, CHANGELOG.md, UPGRADE.md
or docs/ is broken.
Reviewing the base64 bridge by running it turned up four defects, each now
fixed and each with a case in tools/base64-docs/verify.php that fails against
the old code:
- Casting a client-supplied field to string raised "Array to string
conversion" on {"filename": ["a"]}, valid JSON that any client can send. A
default handler logs it on every malformed request and stores the literal
name "Array"; a handler that promotes warnings to exceptions returns 500.
This is what File::__construct() already guards for $_FILES, where the rule
is that remote input must not warn. Both fields are tested with is_string()
rather than cast.
- The data URI header was matched as `data:[^,]*`, which accepts a URI that
is not base64 at all: `data:text/plain,hello world` had its header stripped
and the remainder decoded to seven bytes of rubbish, then written. The
pattern now requires `;base64`, so such a payload fails the strict decode.
- A payload that arrived and could not be decoded was reported as
UPLOAD_ERR_NO_FILE, telling the submitter nothing was uploaded and reading
in getErrorDetails() as the same failure as an absent `data` key. It is now
UPLOAD_ERR_PARTIAL, and the empty case keeps NO_FILE.
- The cap rounded up to four base64 characters, so a 64 byte bound stored a
66 byte file. The encoded check stays, since it is what keeps an oversized
payload from being decoded into a second copy in memory, and an exact check
on the decoded length now follows it.
Checked and left alone: a traversal-shaped client filename (`../../etc/passwd`
stores as `passwd`, inside the tmp directory), the partial-write branch, and
a string, int, null or bool entry, which `??` already handles without warning.
"Turning the defaults off" opened straight into the five calls that disable
the protections the rest of the README argues for, with the consequence of
each compressed into a trailing code comment. It now leads with a warning
callout and a table saying what each call stops applying: that overwriting
drops the reservation but keeps the staged write and the symlink refusal;
that an empty deny-list admits .php, .htaccess and .svg; that setMode(null)
stores world-readable under the usual 022 umask; that
acceptFilesNotUploadedByPhp() gives up move_uploaded_file()'s refusal; and
that allowUnvalidatedUploads() leaves only the storage rules. The code
comments went, since the table carries them.
The isUploadedFile() note now says an override has to assert provenance:
`return true;` gives the check up rather than replacing it, and pairing that
with acceptFilesNotUploadedByPhp() leaves nothing at either end.
"Turning the defaults off" moved to docs/turning-the-defaults-off.md, and is
linked from docs/api-reference.md alone. It is the instructions for disabling
the protections the rest of the README argues for, so it belongs with the
methods rather than in the path of someone reading how to accept an upload. A
reader who wants one of the five arrives through the method they are looking
up; the README's Storage\FileSystem row now says what the class does instead
of pointing at the switches.
The deny-list section keeps array_diff() on getDefaultBlockedExtensions() as
the way to accept one format, which is the answer to "how do I allow SVG".
allowAnyExtension() is no longer named anywhere a reader lands on by
accident.
Reviewing both bridges by running them turned up six more defects, four of
them in the PSR-7 one, which had not had that treatment:
- A field the client nests one level deeper than expected left an array where
an UploadedFileInterface belonged, and the bridge called a method on it:
a fatal Error, where FileList would have raised InvalidArgumentException.
getUploadedFiles() mirrors the multipart field names, so photos[0][0] is
always reachable. The element is tested before it is dereferenced.
- writeTmpFile() returned a bool, collapsing "fopen failed", "over the cap"
and "write failed" into one signal that the caller reported as
UPLOAD_ERR_INI_SIZE. With the tmp directory missing, a 17 KB file under a
1 MB cap was reported to the submitter as too large. It now returns the
UPLOAD_ERR_* that describes what stopped it.
- fwrite() was checked against false, which a short write is not: a full disk
writes fewer bytes than asked and truncates the file rather than failing,
and the truncated file was then validated and stored. Checked against
strlen($chunk), as the base64 bridge already did.
- The copy loop could spin forever. PSR-7 requires neither that read() make
progress nor that eof() ever become true; a stream doing neither read 42
million times in two seconds. Two empty reads in a row now end it.
And two in both bridges:
- fopen() was unsilenced, so a missing tmp directory put the absolute path
into the log on every request. The library silences the same call.
- random_bytes() throws where there is no CSPRNG, and neither bridge caught
it. The library converts its own to STAGING_NAME_FAILED.
Three design gaps are documented rather than coded. $maxBytes bounds one file
and not the batch, so both pages now say to bound the count too; there is no
UPLOAD_ERR_* meaning "too many files", so reporting it would have to misname
it. The cleanup loop belongs in a finally, since nothing else removes a tmp
file. And a base64 payload's key is client input that survives as
getSourceKeys().
Every one has a case in the checkers, each verified to fail against the code
it replaced. The stalled-stream fixture throws after 100 reads rather than
letting the job hang, and the PSR-7 cases run one scenario per try so the
first throw does not mask the rest. Both error handlers honour the @ operator,
which a handler is otherwise called straight through.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jakejackson1
force-pushed
the
docs/readme-4.0-accuracy
branch
from
August 24, 2026 02:57
41b4295 to
485e77d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Docs only — no
src/change, so nothing here alters behaviour.Accuracy
Each of these was checked against the source rather than reasoned about:
ErrorCode.sanitizeForDisplay()'s steps were listed with the trim before the UTF-8 repair. It runs after —sanitizeForDisplay("abc \xC3")returns"abc", not"abc ".isset($file[0])was the wrong warning: a failed transfer does not leave a hole, it renumbers everything after it. With a 3-entry$_FILESwhose entries 0 and 2 failed,count()is 1 and$file[0]is the file the client selected second. Now points atFileList::getSourceKeys().ErrorCodelistedUNKNOWN_TRANSFER_ERRORas one of "the eightUPLOAD_ERR_*outcomes". There are seven; it isforUploadError()'s fallback, whichUPLOAD_ERR_OKalso gets.FileInfoInterfacesent implementers atFileInfo's: FileInfosetter signatures. The interface declares no return type on the three setters — that is what made it implementable in 4.0, and copying the table reintroduced theTypeError.__()with nouse function GravityPdf\Upload\__;, which is exactly the mistake the paragraph below it warns about; under WordPress it silently reaches the global__().sanitizeNameWithExtension()'s real?array $reservedsignature;$displayNameno longer assigned and never used.Added
Filename's public predicates (hasControlCharacters(),hasBidiControls(),deviceComponent(),isReservedDeviceComponent(),extensionComponents(),acceptExtension()), pointed to from "Custom storage backends" — they exist so a custom backend can ask what the shipped one asks, and nothing said so. Plus__call()'sBadMethodCallException, theArrayAccessread/write semantics,FileType::allow()'s empty-side guard and accumulate-not-replace behaviour,Size's parse failure, and thatext-mbstringnow backs thegetErrors()strings too.Fork list
A 3.x list on a 4.0 README: it named
getDirectory()and the upstream import while omittingFileList,uploadValid()and translation, and three bullets described the codebase rather than what a consumer gets. Twelve bullets to seven.One page per bridge
A bridge off the
$_FILESpath now gets a page underdocs/, and the README holds only what every bridge shares — the constructor table,getSourceKeys(), the two provenance decisions,TmpUploadFile, and the tmp-file cleanup (now its own section rather than buried in the PSR-7 walkthrough).docs/psr7.mdtakes the PSR-7 bridge, flattening, cap andmoveTo()warning.docs/base64-uploads.mdis included here and is not my work — it was in the tree uncommitted. It could not be split out: the README's new Bridges table links it, and my relocation rewrites the section it added. Flagging it so it gets reviewed on its own terms.Both checkers read
TmpUploadFileand the cleanup out ofREADME.md, so those two snippets exist once however many bridges ship.tools/psr7-readme/verify.phpreads its own two from the new page;docs/base64-uploads.md's cleanup anchor was repointed.While reviewing the base64 page I found the cap is on
strlen($encoded), which counts line breaks, so a wrapped payload is allowed ~1.3% fewer bytes than an unwrapped one — the page said wrapped payloads decode but not that the wrapping eats the cap, and its own fixture is far enough from the boundary that nothing caught it. Documented.CHANGELOG.mdalso saidUnknown Errorwhere the source saysUnknown error.README is 1023 lines to 961.
Checks
check-syntax,lint,phpstan,phpunit(628 tests, 1 expected skip),psr7-readme,base64-docsall pass against the committed tree. The dist package's top level is unchanged, and no anchor or relative link acrossREADME.mdanddocs/is broken.🤖 Generated with Claude Code