PHPCS code formatting fixes #1534

jom · 2018-06-27T17:48:41Z

1.31.1 (possibly bumping to 1.32.0) has many input/string escaping changes already from #1495. Ideally, none of these will change/break functionality. However, it does require us to do pretty thorough testing for this release. Because of that, I'm proposing these code formatting changes to get things cleaned up.

The vast majority of this PR fixes these common PHPCS errors:

Whitespace changes.
Adds hints for translators: when placeholders are used in strings.
Logic/yoda swaps.
Formatted changes to minimize unnecessary <?php ... ?> blocks.

Additionally, the PHPCS errors that might change some behavior:

Continuing on the escaping work from Adds additional escaping throughout plugin (VIP) #1495.
Remove extract() wherever possible. All uses have been removed except for get_job_manager_template(), where it is difficult to remove.
- I don't anticipate this breaking much functionality. The possible exception to this is the [jobs] shortcode. I'm now using/passing along the cleaned up version of all the attributes. If attributes were mixed/strings before but then are normalized to arrays within the function, those new values are always passed.

This cleans up all errors using the rules in phpcs.xml.dist config file. We are, however, ignoring three rules that will involve more significant code (WordPress.CSRF.NonceVerification.NoNonceVerification) and documentation (Squiz.Commenting.FileComment.Missing and Squiz.Commenting.FunctionComment.MissingParamComment) changes. I'll be removing those from the config post-merge so that new issues are caught in a pre-commit hook (to be added soon).

…n new line

- Strict logic fixes - Some stricter output escaping - Yoda

jom · 2018-06-29T21:19:58Z

includes/admin/class-wp-job-manager-writepanels.php

@@ -253,17 +270,27 @@ public static function input_file( $key, $field ) {
 		}
 		?>
 		<p class="form-field">
-			<label for="<?php echo esc_attr( $key ); ?>"><?php echo esc_html( wp_strip_all_tags( $field['label'] ) ) ; ?>: <?php if ( ! empty( $field['description'] ) ) : ?><span class="tips" data-tip="<?php echo esc_attr( $field['description'] ); ?>">[?]</span><?php endif; ?></label>
+			<label for="<?php echo esc_attr( $key ); ?>"><?php echo esc_html( wp_strip_all_tags( $field['label'] ) ); ?>:
+								   <?php


Auto-fixes in this file look odd.

Per Alex and I's convo

Seems like these should just be echo'd as is

alexsanford

Phew! This was a big one! 😅

alexsanford · 2018-07-01T14:40:56Z

includes/abstracts/abstract-wp-job-manager-form.php

+			)
+		);
+
+		if ( is_wp_error( $response ) || empty( $response['body'] ) ) {


This logic is no longer correct. A non-JSON error will not return a WP_Error here.

Fixed in c7735d5

alexsanford · 2018-07-01T14:48:46Z

includes/admin/class-wp-job-manager-addons.php

+				<a href="<?php echo esc_url( admin_url( 'edit.php?post_type=job_listing&page=job-manager-addons' ) ); ?>" class="nav-tab
+									<?php
+									if ( ! isset( $_GET['section'] ) || 'helper' !== $_GET['section'] ) {
+										echo ' nav-tab-active'; }


Does this really make it through linting? I would think that the } on the same line would be a no-no.

It does, somehow... fixed in a3b335e.

alexsanford · 2018-07-01T14:52:14Z

includes/admin/class-wp-job-manager-admin.php

-					'date_format' => _x( 'yy-mm-dd', 'Date format for jQuery datepicker.', 'wp-job-manager' )
-				) );
+				wp_localize_script(
+					'job_manager_datepicker_js', 'job_manager_datepicker',


These should probably be on separate lines.

Fixed in 2a77dfe

alexsanford · 2018-07-01T15:00:29Z

includes/admin/class-wp-job-manager-cpt.php


 		if ( ! $terms ) {
 			return;
 		}

 		echo "<select name='job_listing_category' id='dropdown_job_listing_category'>";
 		echo '<option value="" ' . selected( isset( $_GET['job_listing_category'] ) ? $_GET['job_listing_category'] : '', '', false ) . '>' . esc_html__( 'Select category', 'wp-job-manager' ) . '</option>';
-		echo $walker->walk( $terms, 0, $r );
-		echo "</select>";
+		echo wp_kses_post( $walker->walk( $terms, 0, $r ) );


I don't know much about what the Walker does. Should we be worried about breaking people's sites because of this sanitization?

Walker, hilariously, just outputs <option> tags. I thought there was a core function for this that we might want to replace this with.

alexsanford · 2018-07-01T15:44:51Z

includes/admin/class-wp-job-manager-settings.php

 		);

-		echo str_replace(' id=', " data-placeholder='" . __( 'Select a page&hellip;', 'wp-job-manager' ) .  "' id=", wp_dropdown_pages( $args ) );
+		echo str_replace( ' id=', " data-placeholder='" . __( 'Select a page&hellip;', 'wp-job-manager' ) . "' id=", wp_dropdown_pages( $args ) ); // WPCS: XSS ok.


XSS is not ok! 😉

We should escape the translated string.

alexsanford · 2018-07-01T17:57:52Z

includes/helper/class-wp-job-manager-helper.php

+			$response = $this->get_plugin_version( $plugin_data['_filename'] );
+			if ( $response ) {
+				// If there is a new version, modify the transient to reflect an update is available.
+				if ( false !== $response


This check is redundant.

Fixed in 235549c

alexsanford · 2018-07-01T22:40:34Z

tests/php/tests/test_class.wp-job-manager-functions.php

+					'search_keywords' => '',
+					'job_types'       => array( 'potato' ),
+				)
+			),
 		);
 		$results['potato_coffee'] = array(


Potato Coffee is not a thing.

alexsanford · 2018-07-01T22:47:54Z

wp-job-manager-functions.php

@@ -492,7 +507,7 @@ function wp_job_manager_notify_new_user( $user_id, $password ) {
 		global $wp_version;

 		if ( version_compare( $wp_version, '4.3.1', '<' ) ) {
-			wp_new_user_notification( $user_id, $password );
+			wp_new_user_notification( $user_id, $password ); // phpcs:ignore


What's going on here? What are we ignoring?

Added the explicit rule. We're skipping the error because the version check catches the change of signature in this function.

7d2a6f5

alexsanford · 2018-07-01T22:55:15Z

wp-job-manager-template.php

 */
 function get_job_manager_template( $template_name, $args = array(), $template_path = 'job_manager', $default_path = '' ) {
 	if ( $args && is_array( $args ) ) {
-		extract( $args );
+		// Please, forgive us.
+		extract( $args ); // phpcs:ignore WordPress.Functions.DontExtract.extract_extract


alexsanford · 2018-07-01T22:58:20Z

wp-job-manager-template.php

-	if( ! empty( $video ) ){
-		// FV Wordpress Flowplayer Support for advanced video formats
+	if ( ! empty( $video ) ) {
+		// FV WordPress Flowplayer Support for advanced video formats.


Most important linter rule...

tripflex

Looks like this commit has broken priority sorting with the force of using intval on priorities ... a lot of users use decimals to allow easily sorting fields, i think we should change this back or use float

tripflex · 2018-07-30T23:30:50Z

includes/abstracts/abstract-wp-job-manager-form.php

@@ -237,10 +237,10 @@ public function get_fields( $key ) {
 	 * @return int
 	 */
 	protected function sort_by_priority( $a, $b ) {
-		if ( $a['priority'] == $b['priority'] ) {
+		if ( intval( $a['priority'] ) === intval( $b['priority'] ) ) {


This breaks priority sorting when using decimals (which is very popular), think we should change this back or switch to floatval instead of intval

tripflex · 2018-07-30T23:31:20Z

includes/abstracts/abstract-wp-job-manager-form.php

 			return 0;
 		}
-		return ( $a['priority'] < $b['priority'] ) ? -1 : 1;
+		return ( intval( $a['priority'] ) < intval( $b['priority'] ) ) ? -1 : 1;


Same as above, breaks priority sorting with decimals

jom added 8 commits June 27, 2018 16:49

Add PHPCompatibility PHPCS rules

3afdc61

Change PHPCS to more closely match Woo

9863c54

PHPCS: Auto formatting fixes for all PHP files

a5e3ae9

PHPCS: Add periods at end of single line comments

3326012

PHPCS: Remove all VIP rules

d817576

PHPCS: Add periods at end of param comments

8bace7d

PHPCS: Add full stop at end of return comments

5204c68

PHPCS: Remove some rules that aren't a priority right now

44a0694

jom added the [Type] Maintenance Changes to non-production files label Jun 27, 2018

jom changed the title ~~[WIP] Beast of a PHPCS fixes~~ [WIP] Safe PHPCS code formatting fixes Jun 27, 2018

jom and others added 20 commits June 28, 2018 11:35

PHPCS: Fix some strange auto-fixes

ebaafc9

PHPCS: Remove usage of extract() in shortcodes

d9259fe

PHPCS: PHP DocBlock fixes for return types and thrown items

a565086

Remove duplicate keys from usage data

ef931d8

Code Smell: Use parent constructor in widgets

999c9c3

Bug: Don't reuse same var names in child foreach loop

59d2400

Merge branch 'master' into change/phpcs-fixes

6688a30

PHPCS: Formatting fix from master merge of wp-job-manager.php

771fbd2

PHPCS: Expanded method args with array, start array new declaration o…

37a2b65

…n new line

PHPCS: Add auto formatting to tests

56abbe7

PHPCS: Auto fixes for tests

c7e8f00

PHPCS: Yoda, docblock, some translation comments

1ae3420

PHPCS: 3rd party code cleanup

73ec587

PHPCS: Abstract class documentaiton cleanup

51ecd6a

PHPCS: Escape LIKE replacement vars correctly in admin CPT

4b30ea0

PHPCS: Admin code fixes

c13231e

- Strict logic fixes - Some stricter output escaping - Yoda

PHPCS: Expand and ignore XSS on pre-escaped setting fields

b35d776

PHPCS: Minor formatting and translation fixes for email notifications

6d914da

PHPCS: Minor fixes for helper and form classes

a0d23bc

PHPCS: Mainly docblock fixes for REST API classes

1b9f6d1

jom added 2 commits June 29, 2018 22:12

PHPCS: Revert some changes in cache helper and cpt

e8aee37

PHPCS: Revert change to not return when echo-ing in template functions

d0c43d6

jom commented Jun 29, 2018

View reviewed changes

jom added this to the 1.31.1 milestone Jun 29, 2018

jom added 2 commits June 29, 2018 22:57

Update renamed callbacks for sending notifications

7aaf50e

PHPCS: Revert change on settings for page input

62d06b7

jom changed the title ~~[WIP] Safe PHPCS code formatting fixes~~ PHPCS code formatting fixes Jul 1, 2018

PHPCS: Fix some whitespace autofix errors in writepanels

7b0d873

jom requested a review from alexsanford July 1, 2018 13:51

jom added 7 commits July 1, 2018 16:35

PHPCS: Add automatic PHPCS checking on pre-commit hook. Mu-ha-ha-ha.

15330f7

PHPCS: Escape translation in settings file

2d58bb7

PHPCS: Collapse some auto-changes for if statements

e058f5a

PHPCS: Remove unnecessary ? true : false casting

cc670d7

PHPCS: Format SQL prepare statements in post types

91fee33

Per Alex and I's convo

PHPCS: Fix issue with auto formatting in html license view

bd79c18

PHPCS: Don't escape before/after widget and before/after title

7658c29

Seems like these should just be echo'd as is

alexsanford reviewed Jul 1, 2018

View reviewed changes

jom added 7 commits July 2, 2018 09:54

PHPCS: Fix issues with changed form validation method

c7735d5

PHPCS: Fix minor issue in addons file

a3b335e

PHPCS: Fix minor issue in admin file

2a77dfe

PHPCS: Minor code formatting fixes

d110743

Remove redundant response check in helper

235549c

PHPCS: Add explicit rule exception for wp_new_user_notification

7d2a6f5

PHPCS: Revert change to selected_category attribute in [jobs]

847b692

jom merged commit 4a50eb2 into master Jul 2, 2018

jom deleted the change/phpcs-fixes branch July 2, 2018 09:26

jom mentioned this pull request Jul 4, 2018

1.31.1 Release #1539

Closed

6 tasks

tripflex reviewed Jul 30, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHPCS code formatting fixes #1534

PHPCS code formatting fixes #1534

jom commented Jun 27, 2018 •

edited

Loading

jom Jun 29, 2018

alexsanford left a comment

alexsanford Jul 1, 2018

jom Jul 2, 2018

alexsanford Jul 1, 2018

jom Jul 2, 2018

alexsanford Jul 1, 2018

jom Jul 2, 2018

alexsanford Jul 1, 2018

jom Jul 2, 2018

alexsanford Jul 1, 2018

alexsanford Jul 1, 2018

jom Jul 2, 2018

alexsanford Jul 1, 2018

alexsanford Jul 1, 2018

jom Jul 2, 2018

alexsanford Jul 1, 2018

alexsanford Jul 1, 2018

tripflex left a comment

tripflex Jul 30, 2018

tripflex Jul 30, 2018

PHPCS code formatting fixes #1534

PHPCS code formatting fixes #1534

Conversation

jom commented Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

alexsanford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tripflex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jom commented Jun 27, 2018 •

edited

Loading