Skip to content

Conversation

@wojsmol
Copy link
Contributor

@wojsmol wojsmol commented Apr 20, 2019

Add a PHPCS ruleset using the new WP_CLI_CS standard.

Fixes #139

Related wp-cli/wp-cli#5179

@wojsmol wojsmol requested a review from a team as a code owner April 20, 2019 21:12
@schlessera
Copy link
Member

mt_rand() is discouraged. Use the far less predictable wp_rand() instead.

=> replace mt_rand() with wp_rand()

Simple placeholders should not be quoted in the query string in $wpdb->prepare(). Found: '%s'.

=> remove quotes around placeholders: '%s' => %s

Global constants defined by a theme/plugin should start with the theme/plugin prefix. Found: "KB_IN_BYTES, ..., ..., ...".

=> wrap in whitelist:

// phpcs:disable WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedConstantFound
if ( ! defined( 'KB_IN_BYTES' ) ) {
	define( 'KB_IN_BYTES', 1024 );
}
if ( ! defined( 'MB_IN_BYTES' ) ) {
	define( 'MB_IN_BYTES', 1024 * KB_IN_BYTES );
}
if ( ! defined( 'GB_IN_BYTES' ) ) {
	define( 'GB_IN_BYTES', 1024 * MB_IN_BYTES );
}
if ( ! defined( 'TB_IN_BYTES' ) ) {
	define( 'TB_IN_BYTES', 1024 * GB_IN_BYTES );
}
// phpcs:enable

(to be continued...)

@schlessera
Copy link
Member

Silencing errors is strongly discouraged. Use proper error checking instead. Found: @preg_match( $search_regex,...

=> Unsure why we have this there, let's leave it for now. Precede with whitelist comment:

// phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged -- Unsure why this is needed, leaving in for now.

Use placeholders and $wpdb->prepare(); found interpolated variable ...

=> Lines 1182, 1184, 1429 - I verified these are escaped, whitelist with a preceding comment:

// phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared -- Escaped through esc_sql_ident/esc_like.

=> Line 1429 - a slightly different comment explanation:

// phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared -- Asserted to be a valid table name through wp_get_table_names.

Object property "$Xxx" is not in valid snake_case format, try "$xxx"

=> We can't change these, wrap block in whitelist:

// phpcs:disable WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSnakeCase -- Property names come from database.
if ( 'PRI' === $col->Key ) {
	$primary_keys[] = $col->Field;
}
if ( self::is_text_col( $col->Type ) ) {
	$text_columns[] = $col->Field;
}
$all_columns[] = $col->Field;
// phpcs:enable

schlessera and others added 9 commits April 21, 2019 03:36
Co-Authored-By: wojsmol <wojsmol@wp.pl>
Co-Authored-By: wojsmol <wojsmol@wp.pl>
Co-Authored-By: wojsmol <wojsmol@wp.pl>
Co-Authored-By: wojsmol <wojsmol@wp.pl>
Co-Authored-By: wojsmol <wojsmol@wp.pl>
Co-Authored-By: wojsmol <wojsmol@wp.pl>
Co-Authored-By: wojsmol <wojsmol@wp.pl>
Co-Authored-By: wojsmol <wojsmol@wp.pl>
Co-Authored-By: wojsmol <wojsmol@wp.pl>
Co-Authored-By: wojsmol <wojsmol@wp.pl>
Co-Authored-By: wojsmol <wojsmol@wp.pl>
@schlessera schlessera added this to the 2.0.2 milestone Apr 21, 2019
@schlessera schlessera merged commit 086db77 into wp-cli:master Apr 21, 2019
@wojsmol wojsmol deleted the use-wp-cli-cs branch April 21, 2019 03:06
@jrfnl jrfnl mentioned this pull request Apr 21, 2019
danielbachhuber pushed a commit that referenced this pull request Nov 18, 2022
Implement CS checking based on the `WP_CLI_CS` ruleset
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adopt and enforce new WP_CLI_CS standard

2 participants