Make WordPress Core

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#43405 closed defect (bug) (fixed)

CS: Fix violations for wp-signup.php

Reported by: garyj's profile GaryJ Owned by: jrf's profile jrf
Milestone: 5.1 Priority: normal
Severity: normal Version: 5.1
Component: Users Keywords: has-patch
Focuses: multisite, coding-standards Cc:

Description

Address code standard violations in wp-signup.php.

Attachments (5)

wp-signup.diff (9.2 KB) - added by GaryJ 7 years ago.
43405.diff (9.2 KB) - added by nikolam 7 years ago.
Applied feedback from @jrf
43405.2.diff (8.1 KB) - added by netweb 7 years ago.
43405.3.diff (7.3 KB) - added by desrosj 6 years ago.
43405.4.diff (7.3 KB) - added by desrosj 6 years ago.

Download all attachments as: .zip

Change History (18)

@GaryJ
7 years ago

#1 @GaryJ
7 years ago

  • Keywords has-patch added

#2 @jrf
7 years ago

  • Version set to trunk

+1

Couple of small remarks/questions (line nrs refer to the "new" lines):

  • Line 122: ( $site_domain ) does not need the parentheses anymore as it is, though it might need output escaping.
  • Line 406/412 - the "Don't override WP globals" sniff is more than anything intended for plugins/themes. Should that sniff be excluded for core ?
  • Line 933: you may want to split this line up as it's pretty long now, i.e. one condition per line.

#3 @netweb
7 years ago

  • Milestone changed from Awaiting Review to 5.0

#4 @SergeyBiryukov
7 years ago

  • Component changed from General to Users
  • Focuses multisite added

@nikolam
7 years ago

Applied feedback from @jrf

@netweb
7 years ago

#5 @netweb
7 years ago

  • Keywords commit added

Patch 43405.2.diff updates 43405.diff and wp-signup.diff with a couple of whitespace indentation issues and removes the instance of phpcs:ignore as it is not required.

#6 @netweb
7 years ago

  • Owner set to jrf
  • Status changed from new to assigned

#7 @pento
6 years ago

  • Milestone changed from 5.0 to 5.1

@desrosj
6 years ago

#8 @desrosj
6 years ago

  • Keywords commit removed

43405.3.diff is a refresh to apply cleanly to trunk. Can someone just give it a review?

#9 @jrf
6 years ago

@desrosj I've given it a once-over. LGTM, just two remarks:

  1. The condition on line 925/939 is still hard to read and confusing for people who don't know operator precedence well enough by heart.

Suggested alternative:

<?php
if ( 'all' === $active_signup
    || ( 'blog' === $_POST['signup_for'] && 'blog' === $active_signup )
    || ( 'user' === $_POST['signup_for'] && 'user' === $active_signup )
) {
    // Code
}
  1. FYI/For future reference: it's likely that we'll be moving away from Yoda conditions in the near future, see https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/1624, but as that hasn't been actioned yet, that shouldn't influence this patch.

#10 @desrosj
6 years ago

  • Keywords commit added

Thanks, @jrf! 43405.4.diff includes your suggestion above.

@desrosj
6 years ago

#11 @desrosj
6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 44626:

Coding Standards: Manually fix coding standards violations in wp-signup.php.

Props GaryJ, jrf, netweb, desrosj.
Fixes #43405.

#12 @desrosj
6 years ago

In 44628:

Privacy: Allow column sorting in the privacy request admin tables.

This allows users to sort the export and erase personal data request tables by “Requester” (post_title, or user email) and “Requested” (post_date, or when the request was created), which can be helpful when sites have many requests present.

Props birgire, ianbelanger, pbiron, desrosj.
Fixes #43405.

#13 @desrosj
6 years ago

  • Keywords commit removed

Sorry for the noise. [44628] does not belong to this ticket.

Note: See TracTickets for help on using tickets.