Skip to content

feat: require reader to set name when commenting#4647

Merged
miguelpeixe merged 6 commits intotrunkfrom
feat/comment-display-name
Apr 9, 2026
Merged

feat: require reader to set name when commenting#4647
miguelpeixe merged 6 commits intotrunkfrom
feat/comment-display-name

Conversation

@miguelpeixe
Copy link
Copy Markdown
Member

All Submissions:

Changes proposed in this Pull Request:

Readers may register with just their email. In this case, their name defaults to the email handle. This should be fine for internal purposes, but shouldn't be used as a public display name in comments.

This PR implements a new input in the comment form, requiring a reader to set their display name if it has not yet been set. Once set, the input should no longer render.

image

The input placement follows the same standard as when commenting anonymously.

Closes NPPM-2701

How to test the changes in this Pull Request:

  1. Sign in as a reader who already has a display name set
  2. Visit an article and confirm there's no new input
  3. In a fresh session, register as a new reader using the registration flow from the header "Sign In" button
  4. Visit an article and confirm the new input renders as in the image above
  5. Confirm you are not able to comment without setting a name first
  6. Set a name, post the comment, and confirm that the filled name is set
  7. Navigate to My Account, verify the account, and confirm the chosen name is saved in the user
  8. Visit another article and confirm the input no longer renders

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@miguelpeixe miguelpeixe self-assigned this Apr 8, 2026
@miguelpeixe miguelpeixe added the [Status] Needs Review The issue or pull request needs to be reviewed label Apr 8, 2026
@miguelpeixe miguelpeixe requested a review from a team as a code owner April 8, 2026 19:40
Copy link
Copy Markdown
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

Tests well! I left one non-blocking comment inline, but otherwise looks good to me.

Other non-blocking feedback from Claude to consider below.

Code Review

Functional Testing — All scenarios pass:

  • ✅ Reader with generic display name sees the "Name *" field in the comment form
  • required attribute blocks empty submission (client-side); server-side wp_die() provides fallback
  • ✅ Server-side validation rejects empty names and email-derived names
  • ✅ Submitting with a valid name updates comment_author, display_name, first_name, and last_name
  • ✅ Field disappears on subsequent pages after name is set
  • ✅ Non-reader users and readers with custom display names don't see the field
  • ✅ PHPUnit: 10/10 pass, 18 assertions

Code Quality — Clean, well-scoped implementation:

  • Good use of preprocess_comment to both validate and override comment_author in one pass — avoids the race where WP reads the stale profile name before wp_insert_comment.
  • Proper sanitization with sanitize_text_field / wp_unslash on $_POST data.
  • Validation covers both generate_user_nicename() and strip_email_domain() patterns.
  • NEWSPACK_ALLOW_GENERIC_READER_DISPLAY_NAMES constant escape hatch (from the existing reader_has_generic_display_name) is a nice touch for sites that don't want this behavior.

Suggestions (non-blocking):

  1. Inline style on input (class-comment-display-name.php:116): style="display:block;width:100%" — the Newspack theme's comment form already styles input[type="text"] to be full-width. Consider removing the inline style or using a CSS class to keep styling in the theme layer. Worth verifying against the block theme as well.

  2. No error handling on wp_update_user() (class-comment-display-name.php:92): If wp_update_user() returns a WP_Error, the comment is still posted with the new name in comment_author but the user profile isn't actually updated. On the next page load, the reader would be prompted again. A minor edge case, but worth noting.

  3. Case-sensitive email-derived name check (class-comment-display-name.php:72-73): The strip_email_domain() comparison is case-sensitive. A reader with email Jane.Doe@example.com whose generic display name is jane-doe (lowercased via generate_user_nicename) could submit Jane.Doe (matching strip_email_domain output) and be blocked, but submitting jane.doe with different casing than the raw email handle would pass. This is a very minor edge case since the intent is to prevent accidental reuse, not strict enforcement — just flagging for awareness.

Overall, this is a clean, focused PR with good test coverage. Nice work! 👍

@miguelpeixe miguelpeixe requested a review from dkoo April 9, 2026 15:14
Copy link
Copy Markdown
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

Tests well! I left one non-blocking inline comment below, otherwise looks good to me. ghost message leftover in the browser, sorry

@github-actions github-actions bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Apr 9, 2026
@miguelpeixe miguelpeixe merged commit db5ad73 into trunk Apr 9, 2026
11 checks passed
@miguelpeixe miguelpeixe deleted the feat/comment-display-name branch April 9, 2026 15:32
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 9, 2026

Hey @miguelpeixe, good job getting this PR merged! 🎉

Now, the needs-changelog label has been added to it.

Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label.

If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label.

Thank you! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Status] Approved The pull request has been reviewed and is ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants