Skip to content

Re-land the WPFTP feedback export with the review fixes - #3661

Open
maciejpilarski wants to merge 1 commit into
trunkfrom
fix/wpftp-feedback-export
Open

Re-land the WPFTP feedback export with the review fixes#3661
maciejpilarski wants to merge 1 commit into
trunkfrom
fix/wpftp-feedback-export

Conversation

@maciejpilarski

@maciejpilarski maciejpilarski commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Re-lands #3651 (reverted in #3660) with every finding from the post-merge review applied. The feature is unchanged in purpose: a Tools > WPFTP Feedback page and a wp feedback export command that export Jetpack Forms responses together with the logged-in user who submitted each one, which Jetpack's own CSV export omits. Original work by @Piyopiyo-Kitsune; this PR only changes what the review found.

#3660 is merged, so trunk holds none of the original code; this diff is the three plugin files plus two require_once lines, and nothing here lands until it is approved.

Context

The two WordPress Facilitator Training Program courses run a Jetpack Forms survey asking respondents what role they are in. Matching those self-reported roles against course completions and credentials needs the WordPress.org account behind each response. The review (ten finder angles, verified against the Jetpack Forms source, the shared Export_CSV mu-plugin utility and WordPress core) found that the merged version altered the exported data and let the CLI widen an export silently. Details in #3660.

What changed, by finding

Export content

  • Both date bounds are inclusive. date_query now carries inclusive => true, so a Y-m-d range covers its first and last day and a one-day range works.
  • Values are written verbatim. The module writes its own CSV with fputcsv, applying only Jetpack's Contact_Form_Plugin::esc_csv() leading-character guard against formula injection. The shared WordPressdotorg\MU_Plugins\Utilities\Export_CSV is no longer used: its per-cell sanitize_text_field() collapsed paragraph breaks, encoded < and stripped %xx, and its escaping inserted apostrophes before -, +, @ and = inside ordinary prose. The response now also sends Content-Type: text/csv; charset=utf-8, as Jetpack's export does.
  • Headers keep Jetpack's space prefix, and the added columns carry the same prefix ( Status, Logged-in username, Logged-in display name, Logged-in user ID, Logged-in user recorded), so they can never collide with a form field label.
  • A Status column tells spam and trash rows apart when those boxes are ticked.
  • A "Logged-in user recorded" column. Jetpack only began capturing the submitter's account in forms package 7.14.0 (March 2026). Older responses have no logged_in_user key in their stored content, so an empty username alone cannot tell "not captured" from "not logged in". The column reads whether the key exists.
  • The ID column key is built the way Jetpack builds it, ' ' . __( 'ID', 'jetpack-forms' ), with a fall-back to the first column, instead of matching the English literal.
  • An all-test-response batch is an empty result, not a WP_Error, so the "No feedback responses found" notice shows instead of a wp_die().

Arguments that used to widen the export

  • status values are checked against Jetpack's four real statuses (publish, draft, spam, trash); anything else is rejected. WP_Query drops the status clause entirely when no requested status is registered, which exported every row.
  • after / before must parse; an unparseable bound previously became 1970-01-01 and matched everything.
  • --post must be numeric; a non-numeric value previously cast to 0, meaning all forms.

CLI safety

  • --dir is required, must exist and be writable, and is refused if it lies inside ABSPATH or WP_CONTENT_DIR, since the file holds usernames and IP addresses under a predictable name and the current directory was often the web root.
  • The file is created with fopen( ..., 'x' ) and chmod 0600, so it never overwrites and is owner-readable only.
  • The filename names the form (or all-forms) and the time to the second.
  • An empty result is a warning with exit code 0, so scheduled runs can tell "nothing this window" from a failure.

Capability and scale

  • The page and handler are gated on a new export_feedback_responses capability, granted to administrators in inc/capabilities.php alongside the plugin's other custom caps. It can be added to a role explicitly to give a facilitator access without full administrator. The core export capability no longer opens a de-anonymising export.
  • Responses are processed in batches of 200 with Feedback::clear_cache() between batches and _prime_post_caches() per batch, and the CSV streams to output rather than being built as one string.
  • The form picker covers every status the export can include (a form whose responses were all marked spam can now be selected), primes the post cache once instead of one query per option, and labels every option with its post ID, which is also the value --post takes. Untitled jetpack_form parents get a (jetpack_form) label instead of a blank one.

Not changed: the form filter still keys on the feedback's post_parent. Jetpack's newer source-page metadata would need its SQL union filter; the ID in each picker label makes the right value visible in the meantime.

Testing

  • php -l clean on all three files.
  • phpcs clean against this repo's phpcs.xml.dist (WordPress-Core, Docs, Extra with the repo's exclusions). The one phpcs:ignore is on the jetpack-forms text domain, which has to match Jetpack's own key.
  • I do not have a Learn wp-env with Jetpack Forms responses locally, so the runtime path has not been exercised end to end here. Please run the Tools page and wp feedback export --dir=/tmp in wp-env before merging; the original PR's manual test plan still applies, plus:
    • A one-day date range (same date in both fields) returns that day's responses.
    • A textarea answer with paragraph breaks and a - after a space comes through unchanged.
    • wp feedback export --status=all --dir=/tmp and --after=nonsense error out instead of exporting.
    • wp feedback export --dir=. from the site root is refused.
    • Ticking "Include spam" produces rows whose Status column reads spam.
    • An administrator sees Tools > WPFTP Feedback; an editor does not.

🤖 Generated with Claude Code

Re-adds the Tools > WPFTP Feedback page and the `wp feedback export`
command from #3651, reverted in #3660, with the post-merge review applied:

- Date bounds are inclusive and must parse.
- The CSV is written verbatim with fputcsv and Jetpack's own esc_csv()
  guard instead of the shared Export_CSV utility, which rewrote free text.
- Status values are validated, --post must be numeric, and an all-test
  batch is an empty result rather than an error.
- New columns: Status and "Logged-in user recorded" (Jetpack only began
  capturing the submitter in forms package 7.14.0); added columns carry
  Jetpack's space prefix so they cannot collide with form fields.
- CLI: --dir is required and refused inside ABSPATH or WP_CONTENT_DIR,
  the file is created with 'x' and chmod 0600, the name carries the form
  and the time, and an empty result exits 0 with a warning.
- A dedicated export_feedback_responses capability, granted to
  administrators, replaces core's export.
- Responses are processed in batches of 200 with the Feedback cache
  cleared between them; the form picker covers every status and shows IDs.

Co-authored-by: Destiny Kanno <17694715+Piyopiyo-Kitsune@users.noreply.github.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

CLI validation can still widen an explicit post filter, and file permissions are not applied atomically.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds secure Jetpack Forms feedback exports with submitter identity through wp-admin and WP-CLI.

Changes:

  • Adds batched CSV generation with status, date, form, and user metadata.
  • Adds an administrator capability and Tools page.
  • Adds a CLI export command with output-directory safeguards.
File summaries
File Description
wporg-learn.php Loads the export modules.
feedback-logged-in-user-export.php Implements querying, CSV generation, and admin UI.
class-feedback-export-cli.php Implements the CLI export and file handling.
capabilities.php Grants administrators the export capability.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +100 to +101
// 'x' refuses to open an existing file, so nothing is ever overwritten.
$handle = @fopen( $path, 'x' ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_fopen, WordPress.PHP.NoSilencedErrors.Discouraged
Comment on lines +72 to +74
if ( null !== $post && ! ctype_digit( (string) $post ) ) {
\WP_CLI::error( sprintf( '--post must be a numeric post ID, "%s" given.', $post ) );
}
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