Sponsors: store agreement attachments with the private status by KokkieH · Pull Request #1992 · WordPress/wordcamp.org

KokkieH · 2026-09-02T14:33:47Z

Makes sponsor agreement attachments private, rather than letting them inherit their sponsor's status, and gives them a random suffix in the name as wordcamp-payments does for its own files. A sponsor's other uploads — its logo, images in its content — are left alone.

An agreement uploaded from the Sponsor screen is named and made private in the request that creates it, so nothing has to be moved afterwards. One chosen from the existing Media Library is handled when the sponsor is saved, which does rename the file it already had.

Adds a one-time wp wc-sponsor-agreements migration for agreements attached before this, to be removed once it has been run. It sets the status and records that the file still needs renaming; renaming is being handled separately.

Testing

On an event site, e.g. https://seattle.wordcamp.test/2023/, as an admin.

Sponsors → Add New, use Attach Signed Agreement to upload a PDF. Publish.
Reload. View Agreement opens the file, and its name has a 16-character suffix.
wp --url=<site> eval 'echo get_post( <ATTACHMENT_ID> )->post_status;' → private.
Repeat with a JPG or PNG as the agreement. Same result — this is the path that depends on the browser sending the field, so it's the one worth checking after any change to wcb-spon.js.
On the same sponsor, set a featured image and add an image to the content. Both stay inherit under the names they were uploaded with.
Upload a PDF through Media → Add New, then attach it with Attach Signed Agreement and press Update. It becomes private and is renamed on save.
The sponsor is unchanged: it renders on the front end, and /wp-json/wp/v2/sponsors/<SPONSOR_ID> still reports "status":"publish".
Media → Library still lists and opens every agreement above for an admin or editor.

The migration, for agreements attached before this:

wp wc-sponsor-agreements scan --verbose reports which sites have any.
wp --url=<site> wc-sponsor-agreements backfill --dry-run, then without --dry-run. Each is reported Private: yes, a second scan reports none left, and View Agreement still opens the file. The files keep their names — they are recorded for the separate rename pass with _wcorg_sponsor_agreement_needs_rename.

One site per process, so across the network:
```
wp wc-sponsor-agreements scan | while read -r site; do
    wp --url="$site" wc-sponsor-agreements backfill
done
```

Automated: phpunit --testsuite "WordCamp MU Plugins" --group sponsor-agreements (45 tests). The full WordCamp MU Plugins, WordCamp Post Types and WordCamp Payments suites pass, apart from failures that also occur on production — which ones varies by environment.

- Mark the attachment a sponsor names as its agreement, and give it the `private` status rather than the `inherit` it gets from the Media modal - Leave marked attachments out of the XML-RPC and admin-ajax media details for users who can't read them - Add a random suffix to the names of files uploaded to a sponsor Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QJTGFnceNhPYJGb2F32T5r

- Add `wp wc-sponsor-agreements scan` and `backfill`, to give agreements attached before the previous commit the same status and file name - Load the command from the WP-CLI bootstrap Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QJTGFnceNhPYJGb2F32T5r

mcliwanow · 2026-09-02T15:24:10Z

I tried the backfill against a 3000x3000 JPEG agreement (a phone photo of a signed page is probably the most common case here). WP stores that as foo-scaled.jpg, keeps the original as original_image = foo.jpg, and names the sizes foo-300x300.jpg ... foo-2048x2048.jpg. rename_agreement_file() takes $old_base from the attached file, so it's foo-scaled, none of the sizes match it, and original_image is never looked at. The command says Renamed: yes, but only the -scaled file moved. The original and all seven sizes still have their old names, and the 1024/1536/2048 ones are perfectly readable copies of the contract.

PDFs and images under 2560px are fine, their generated files start with the same base name.

I suggest using original_image as the base when it's set, renaming that file too, and swapping the leading base in every size. test_an_existing_agreement_is_renamed() uses a fake PDF with no sizes, so a big image fixture would catch this. Worth fixing before the run, the command is gone afterwards.

- Take the new base name from `original_image`, so that a scaled image's full-size original and its sizes move along with the attached file - Require the name separator before treating a file as a derivative - Count each file, and report an agreement as unfinished when any of them is left behind or its file is already gone - Render the report with `format_items()`, keeping the non-table formats parsable on STDOUT and the summary on STDERR Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QJTGFnceNhPYJGb2F32T5r

KokkieH · 2026-09-03T12:01:41Z

@mcliwanow Image-size files should now be handled correctly by the backfill. That's a good catch!

bor0

Focused pass over the CLI, as asked.

The shape is right. The direct-table scan that avoids switch_to_blog() is a good call, the reasoning about ms_upload_constants() matches what the code actually does, and rename_agreement_file() handles the genuinely awkward parts carefully -- scaled originals, two sizes naming one file, the - separator that tells photo-150x150.jpg from photobooth-150x150.jpg -- each with a test pinning it.

Everything below is in the command layer rather than in the helpers, and I don't think that's a coincidence: Command::scan() and Command::backfill() are the only things in this file with no test coverage, while every function they call has some. The ordering issue at L398-399 would have surfaced the moment anyone wrote a test that runs backfill twice.

Ranked:

L398-399 -- the rename half isn't resumable. Setting the status first removes the attachment from the only query that can ever find it again. This contradicts "Safe to re-run" in the docblock and in the PR description. The one I'd call blocking.
L359 -- --skip-rename makes that permanent for every agreement on a site, and reports success while doing it.
L310 -- a failed query reads as a clean site, so scan can under-report and nothing in the output says so.
L206 -- a crash mid-loop leaves disk and DB out of step, on a command whose own docblock cites memory pressure on the sandbox as a known problem.
L108 -- file missing conflates an absent file with a misconfigured run, which is the exact mistake the one-site-per-process rule exists to prevent.
L393 -- --dry-run doesn't enumerate anything, and no run names the site it acted on.

None of this is wrong on the happy path. It's the recovery and reporting paths, which for a one-shot migration across the network are the ones that decide whether a bad run is noticed and fixable. I'd want a couple of tests over backfill() itself -- run twice, and run with a rename that fails -- before this goes near the sandbox.

bor0 · 2026-09-03T12:06:18Z

+				$migrated = make_agreement_private( $agreement_id );
+				$outcome  = $skip_rename ? $untouched : rename_agreement_file( $agreement_id );


The status change lands before the rename, and get_public_agreement_ids() only returns post_status = 'inherit' rows -- so once make_agreement_private() has succeeded, no later backfill can see this attachment again.

That makes the rename half a single-attempt operation, which the docblock above ("Safe to re-run: a site with nothing outstanding does one query and stops") and the PR description ("Idempotent and resumable") both promise it isn't. Concretely: a run reports 1 moved, 3 left, the operator fixes whatever caused it, re-runs, and gets No sponsor agreements left to migrate. The three files keep the names they had, and nothing is left that can pick them up.

Swapping the two lines fixes it without any extra state -- rename first, set the status only once the rename came back clean. A crash between the two then leaves the row inherit, so the next run redoes both; worst case is a second suffix on an already-renamed file, which is harmless and visible.

bor0 · 2026-09-03T12:06:18Z

+		WHERE sponsor_meta.meta_key IN ( $key_placeholders )
+		AND sponsor.post_type IN ( $type_placeholders )
+		AND agreement.post_type = 'attachment'
+		AND agreement.post_status = 'inherit'",


The other half of the resumability point at L398. inherit is the only thing that makes an attachment visible to this migration, and backfill() clears it on the first pass regardless of what happened on disk afterwards.

If the ordering in backfill() stays as it is, this query needs a second arm -- attachments already carrying _wcorg_sponsor_agreement whose file still has no suffix -- otherwise the command gets exactly one attempt per attachment and no way to tell, later, which attempts fell short.

bor0 · 2026-09-03T12:06:18Z

+	 * [--dry-run]
+	 * : Report what would change, without changing it.
+	 *
+	 * [--skip-rename]


Worth reconsidering while the ordering at L398 stands: --skip-rename sets the status on every agreement on the site and touches no file, which by the query at L64 permanently forecloses the rename for all of them -- and the run ends on %d agreements processed., i.e. reports success.

If it's here for a staged rollout, inverting it (--status-only being the arm that can't run first) would keep the option safe. Otherwise --dry-run already covers the look-before-you-leap case and this could just go.

bor0 · 2026-09-03T12:06:18Z

+	);
+	$attached_path = get_attached_file( $attachment_id );
+
+	if ( ! $attached_path || ! is_file( $attached_path ) ) {


This early return yields renamed: 0, left_behind: 0, which describe_rename() renders as file missing. Accurate for a genuinely absent file -- but it is also exactly what the operator sees when the process is pointed at the wrong uploads directory, i.e. the ms_upload_constants() / UPLOADS trap the docblock warns about. In that case every row reads Private: yes / file missing, the summary is a plain success, and nothing distinguishes it from a site whose files were already gone.

Two cheap separations: carry $attached_path out in the outcome so the report can print what it looked for, and have backfill() assert up front that wp_upload_dir()['basedir'] belongs to the site it is about to migrate. The whole one-site-per-process rule currently rests on a comment; four lines would make it rest on the code.

bor0 · 2026-09-03T12:06:18Z

+		$found   = 0;
+
+		// A site whose tables have gone is a broken row, not a reason to stop part-way through a network.
+		$wpdb->suppress_errors( true );


get_col() returns array() when the query fails, so with errors suppressed a broken site is indistinguishable from a site with nothing to migrate. It isn't just skipped -- it is counted as clean in the %d of %d sites have agreements to migrate. summary, and its URL never reaches the pipe that drives backfill.

On a network this size that's the failure mode least likely to be noticed. Reading $wpdb->last_error after each call and reporting the sites that errored on STDERR would keep the "a broken row isn't a reason to stop" behaviour while making the gap visible.

Minor, same call: suppress_errors() returns the previous value. Capturing it and restoring that at L334, rather than forcing false, keeps this from changing state it didn't own.

bor0 · 2026-09-03T12:06:18Z

+		$metadata['file'] = $path_prefix . $new_attached;
+	}
+
+	if ( $metadata ) {


The metadata and _wp_attached_file are written once, after every @rename() has already happened. If the process dies inside the loop -- and the scan docblock cites real memory pressure on the production sandbox -- the files sit on disk under their new names while the DB still points at the old ones. The attachment stops resolving, the files are orphaned, and the mapping between them exists nowhere.

Moving these two writes inside $move(), or flushing after each successful rename, narrows the window to a single file. If that's more churn than you want in a file that's being deleted anyway, the deployment notes should at least carry a recovery procedure, because reconstructing the pairing afterwards means matching files to attachments by hand.

bor0 · 2026-09-03T12:06:18Z

+			'left_behind' => 0,
+		);
+
+		foreach ( $agreement_ids as $agreement_id ) {


Two things that would help on a run this wide:

--dry-run reports pending and nothing else. For a command that renames files across the network, a dry run that doesn't name the files it intends to move (attached file, original_image, each size) or the base it would give them is hard to sign off on -- and rename_agreement_file() already computes all of that before it touches anything.

Nothing in the output names the site being migrated. --url is easy to forget, and forgetting it quietly targets the main site instead. Printing home_url() above the table would make each run self-documenting, which matters when that output is the only record the operator keeps.

Also worth a WP_CLI::confirm() on the non-dry-run path with the usual --yes escape, given what the command does.

- Drop the file renaming, `--skip-rename`, and the `Renamed` column, so that the migration reads and writes the database only - Report the sites `scan` could not read, rather than counting them clean - Name the site each `backfill` run acted on Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QJTGFnceNhPYJGb2F32T5r

KokkieH · 2026-09-04T09:47:32Z

Latest commit narrows the migration to setting the attachment status. The file renaming has been split out and will be handled separately, so this can be deployed and the status backfill run while that's still being worked out.

@bor0 that removes most of what your review covered — the ordering, --skip-rename, the crash window between disk and database, and the uploads-directory question all go with it, since the command now reads and writes the database only. The two that still applied are fixed: scan reports the sites it couldn't read instead of counting them clean, and each run names the site it acted on.

…uploaded - Move the CSPRN from `wp_unique_filename` to the meta hook, so that a file picked from the Media Library is covered and a sponsor's other uploads are not renamed - Restore `rename_agreement_file()`, called after the status is set and never fatal - Record the attachments the migration acts on, as the work list for renaming the files attached before this Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QJTGFnceNhPYJGb2F32T5r

- Name the file at `wp_unique_filename` and set the status at `wp_insert_attachment_data`, so an agreement is never written under the name it arrived with or with the status of its sponsor - Treat any PDF uploaded against a sponsor as an agreement, whether or not the modal marked it - Drop the `wp_prepare_attachment_for_js` filter that did this afterwards Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

- Log `sponsor_agreement_files_not_renamed` when `rename_agreement_file()` couldn't move everything - Correct the docblock claim that nothing links to an agreement by URL Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

- Move the meta key to `sponsor-agreements.php` and name it for what it says about the file rather than for what wrote it - Set it from `secure_agreement()` too, so a rename that left files behind is findable and not only logged Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

bor0 · 2026-09-17T10:37:25Z

Took another pass over the branch, now at 331b51e9.

Ran the suites locally:

phpunit --testsuite "WordCamp MU Plugins" --group sponsor-agreements - 45/45 pass.
Full WordCamp MU Plugins suite - 417 tests, one failure (Test_QuickBooks_OAuth_State::test_authorize_url_carries_the_stored_state). It fails the same way on production, so it is not from this branch. Thanks for dropping the test name from the description, that is better than either of us guessing which one the next person will hit.

Also spot-checked that each fix in the last commit has a test behind it: reverting any one of the three on its own fails exactly one test. It does.

My notes on rename_agreement_file() are addressed. I have a couple of small follow-on points about the deferred file work, left with the team in the usual place along with the rest of the review, since they are easier to discuss with the full context to hand. Neither is a change to this branch.

Nothing blocking from me.

- Count an edited image's pre-edit files, recorded in `_wp_attachment_backup_sizes`, as left behind, without moving them - Record an attachment whose file is not on disk rather than treating it as finished - Count files the metadata names that aren't the attachment's own apart from `left_behind`, so a row can come off the list Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

bor0

Approving at 331b51e9.

My notes on rename_agreement_file() are all addressed, and each fix has a test that fails when that fix alone is reverted. I checked that rather than taking it: reverting any one of the three fails exactly one test.

Suites on the branch: 45/45 in --group sponsor-agreements, and 417 in the full WordCamp MU Plugins suite with only the QuickBooks failure that fails the same way on production. CI is green on 8.3, 8.4 and 8.5.

I have two small follow-on points about the deferred file work, left with the team in the usual place. Both are notes for whoever picks that up rather than changes to this branch, so they are not holding anything here.

Worth waiting on @mcliwanow before merging, since they raised the scaled-image case that led to the rewrite and had the last word on the approach.

- Skip backup-size entries that aren't arrays, rather than relying on `??` - Say that `rename_agreement_file()` runs once per attachment, and that a work-list row can be stale by the time it's read Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

mcliwanow

The scaled-image case is handled and tested, and the upload path on wp_unique_filename / wp_insert_attachment_data / add_attachment is what I had in mind. Checked against core trunk that wp_ajax_upload_attachment() runs the nonce, upload_files and edit_post checks before any of those fire. Approving.

KokkieH and others added 2 commits September 2, 2026 16:33

KokkieH marked this pull request as ready for review September 2, 2026 14:50

KokkieH requested review from bor0 and mcliwanow September 2, 2026 14:50

KokkieH and others added 2 commits September 3, 2026 13:45

Merge branch 'production' into sponsors/private-agreement-attachments

6d351ba

bor0 reviewed Sep 3, 2026

View reviewed changes

KokkieH and others added 5 commits September 4, 2026 12:41

Merge branch 'production' into sponsors/private-agreement-attachments

d364d27

KokkieH requested a review from bor0 September 17, 2026 06:26

mcliwanow mentioned this pull request Sep 17, 2026

Budgets: store payment files with the private status and delete them with their request #2086

Merged

bor0 approved these changes Sep 17, 2026

View reviewed changes

mcliwanow approved these changes Sep 18, 2026

View reviewed changes

Merge branch 'production' into sponsors/private-agreement-attachments

b70de1d

KokkieH merged commit 6042b5f into production Sep 18, 2026
5 checks passed

KokkieH deleted the sponsors/private-agreement-attachments branch September 18, 2026 09:14

KokkieH mentioned this pull request Sep 22, 2026

Sponsors: include central and unrecorded PDFs in the agreement backfill #2098

Merged

		$migrated = make_agreement_private( $agreement_id );
		$outcome = $skip_rename ? $untouched : rename_agreement_file( $agreement_id );

Conversation

KokkieH commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Uh oh!

mcliwanow commented Sep 2, 2026

Uh oh!

KokkieH commented Sep 3, 2026

Uh oh!

bor0 left a comment

Choose a reason for hiding this comment

Uh oh!

bor0 Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

bor0 Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

bor0 Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

bor0 Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

bor0 Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

bor0 Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

bor0 Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

KokkieH commented Sep 4, 2026

Uh oh!

bor0 commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bor0 left a comment

Choose a reason for hiding this comment

Uh oh!

mcliwanow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

KokkieH commented Sep 2, 2026 •

edited

Loading

bor0 commented Sep 17, 2026 •

edited

Loading