[gonzaloebiz]

Comment only. One sentence, in the setSurface() docblock merged yesterday.

What changed

- * pattern below therefore bounds what may be carried, and the receiver
- * validates independently, because it is reachable without us.
+ * pattern below therefore bounds what may be carried, and the receiver
+ * validates again rather than trusting that this happened.

The clause described what can reach the receiver. It was there to justify why the receiver checks at all, and it does not need to be: the reason either end validates is that neither can assume the other did. That version says the same thing and describes nothing.

Why it is worth a PR of its own

This file is not internal. It installs on every merchant's server as part of the package, and the repository is public, so a sentence in a docblock here is published twice over and stays published. The same sentence appeared in the description of the PR that merged it; that has been edited, but an edit only changes the default view, and source is worse than a description because it also ships.

Nothing about the rule changes — the pattern, the order of the checks and what they reject are untouched. Gates green: tools/tests/test-surface-token.php 31 checks, tools/no-deprecated-calls.php 64 files 0 findings.

[dipola]

Correct, and the reasoning in the description is better than the one I gave you. "Source is worse than a description because it also ships" is the part I had not said: a description is edited in one place, and /.gitattributes makes clear that src/ lands in every store's vendor directory while /tools does not. That asymmetry is the right lens for the whole question.

Approving. One thing before you consider it closed, though: this is one sentence of several in the same file, and your own packaging rule sorts them for you.

The test worth applying, rather than a list to strip

The rule is not "do not mention the far end". A client that failed to document the limits it is coded against would be a worse client. The distinction is:

• Contract facts — keep. Things the library must know to behave correctly, and that any caller would discover by using the endpoint: there is a size ceiling, an oversized roster is refused whole rather than truncated, a 2xx does not mean the report was used. MAX_BYTES, MAX_AUDIENCES_BYTES and the roster-capping comments are all in this class and should stay. They explain constants that would otherwise look arbitrary.
• Construction facts — drop. Things that describe how the far end is built or what it is exposed to. These explain nothing about this library's behaviour, because the behaviour would be identical whatever the answer was.

By that test, in src/ (i.e. shipped)

The one I would fix next, and it is the clearest case in the file: the \z docblock currently names the other end's implementation language while justifying the anchor. The decision stands entirely on its own without it — PHP's $ also matches before a final newline, so \z is the anchor that means what it looks like. That is the whole argument, it is a fact about PHP, and it is checkable by the reader in front of them. The clause after it adds nothing to the rule and is pure construction.

Two worth a lighter touch:

• The MAX_SURFACE_BYTES docblock attributes the number rather than stating it. "128 is the ceiling a value has to fit for it to land" is the same guidance for a maintainer picking a different number later, without saying whose ceiling it is or that it is enforced elsewhere.
• The sfail docblock enumerates a reject taxonomy in order to explain why rejections cannot reach that counter. The load-bearing half — a 2xx does not mean the report was used, so this counter is transport failure only — is a contract fact and worth keeping. The enumeration behind it is not needed for the reader to trust the conclusion.

tools/tests/test-surface-token.php also carries one, but /tools export-ignore means it never reaches a merchant's vendor directory, so it is public-repo-only and the least urgent of the four.

Not a PR each

Worth doing as one pass rather than one PR per sentence — same class, same file, no behaviour anywhere in it. The gates you already run cover the risk, since none of this touches a line of logic.

[gonzaloebiz]

All four done in this PR rather than four of them — same class, same file, no logic in any of it. CI green.

Your test is the part worth keeping, so I wrote it into the commit message rather than leaving it in a thread: a contract fact is something this library must know to behave correctly and that any caller would meet by using the endpoint; a construction fact describes how the far end is built or what it is exposed to, and explains nothing here, because this library would behave identically whatever the answer was.

I checked your packaging premise rather than taking it — .gitattributes is /.github, /tools and /.gitattributes as export-ignore, and the file's own comment says why: composer takes its dist from the repository archive, so anything not excluded lands in every store's vendor directory. So src/ ships and tools/ does not, and the three in src/ are the urgent ones.

The \z anchor. You were right that it was the clearest case, and dropping the clause made the argument shorter rather than weaker:

\z is the anchor that means what it looks like. A rule anchored at $ accepts a token no dispatch ever produced, differing from one that a dispatch did by a byte that does not print.

That is a fact about PHP, checkable by the reader in front of them, and it says why the odd-looking escape is there — which was the whole reason the sentence existed.

MAX_SURFACE_BYTES. Took your phrasing. Added one line for the maintainer who comes to change it: move the bound, not the fit — since the paragraph above it lists real route lengths and could otherwise read as an invitation to tune it down to 81.

sfail. Kept the load-bearing half, dropped the enumeration. The conclusion never needed it: a 2xx says the send arrived, not that the report was used is what makes "declines cannot reach this counter" follow.

The test comment, for completeness, even though /tools export-ignore means it was public-repo-only.

Grepped the shipped source afterwards for all four phrasings — nothing left.

One thing I did not touch, in case you would have: MAX_BYTES, MAX_AUDIENCES_BYTES and the roster-capping comments. By your test they are contract — they explain constants that would look arbitrary otherwise, and a caller meets every one of them by sending. Say so if you read any of them differently.

[dipola]

Checked the merged develop rather than the diff: the four phrasings are gone from src/, and the seven mentions that remain all pass the test. I read MAX_BYTES, MAX_AUDIENCES_BYTES and the roster comments exactly as you do — keep them.

They earn their place twice over. A caller meets every one of them by sending, so none of it is anything we are telling them. And each explains a constant that would otherwise look arbitrary: without "refused whole rather than truncated", the local roster cap reads as a preference rather than the reason it exists, and the next maintainer raises it.

The one closest to the line, for the record rather than as a request: the contact_opt_out comment, which says the flag travels so a refusal can be told apart from a request that happened not to carry the pair. It describes a distinction being drawn at the far end. It stays, because the alternative is a flag with no stated purpose — and the fact it rests on, that absent and false are not the same answer, is one a caller has to know to send correctly. That makes it contract under your own definition, and it would only become construction if it started explaining what is done with the distinction rather than why the flag is sent.

Two things you did here that were better than the review:

Verifying the packaging premise instead of taking it. I asserted src/ ships and tools/ does not; you read .gitattributes and confirmed it, including the reason. That is the right instinct on anything a review hands you — I have been wrong twice today on things I stated confidently in this repo.

Putting the test in the commit message. A thread is not where a rule survives. The next person deciding whether a sentence belongs in a public docblock will find it in git log and not in a PR nobody re-reads.

One note on scope, since the leak question is not actually closed by this merge: develop is clean, but the published package is still 3.0.51, which predates all of it. Nothing changes for an installed store until the next tag. That is an argument for cutting one sooner rather than later, not for doing anything more here.