Improve Wpcs

[widoz]

Strictly comparisons
Remove extra unneeded new line separators for readability
Grouped assignments for readability

[stevengliebe]

Thanks for pointing out the comparisons. I'll fix those and look at the $option_prefix.

I'm not so sure about removing lines though. Are you checking against an additional standard? I'm using the WordPress standard which includes core, VIP, etc. It doesn't warn me about removing lines like in your PR and I don't see it in the standards on make.wordpress.org.

I find this to be a lot more readable:

function wie_security_notices() {

  $notices = array();

  // Outdated PHP notice.
  $notices[] = 'wie_php_notice';

  // HTTP notice.
  $notices[] = 'wie_http_notice';

  // Filter notices.
  $notices = apply_filters( 'wie_security_notices', $notices );

  // Loop notices to activate.
  foreach ( $notices as $notice ) {
  	add_action( 'admin_notices', $notice );
  }

}

...than this:

function wie_security_notices() {

  $notices = array();
  // Outdated PHP notice.
  $notices[] = 'wie_php_notice';
  // HTTP notice.
  $notices[] = 'wie_http_notice';
  // Filter notices.
  $notices = apply_filters( 'wie_security_notices', $notices );

  // Loop notices to activate.
  foreach ( $notices as $notice ) {
    add_action( 'admin_notices', $notice );
  }
}

I do see the standard for new lines on switch but in that case I'll stick with what to me is more readable. It might be better to change that to if. Most other warnings I get about new lines seem to be false positives because they are not consistent.

Other than that, there are warnings about $_GET, $_POST, etc. There must be some better way to deal with handling that data. Maybe in the next release. Any ideas?

[widoz]

Hi,

Thank you for your response.

I'm not so sure about removing lines though. Are you checking against an additional standard? I'm using the WordPress standard which includes core, VIP, etc. It doesn't warn me about removing lines like in your PR and I don't see it in the standards on make.wordpress.org.

Any additional standard. Generally multiple assignments are grouped but in this case the additional comment per assigment may decrease readability as you pointed out. I think, depending on dev habit. Anyway nothing to strictly related to Wpcs.

I do see the standard for new lines on switch but in that case I'll stick with what to me is more readable. It might be better to change that to if.

I don't think using if over switch may increase readability, instead when checking for the same single value a switch is the right choice, only have a lot's of empty lines around the condition blocks may decrease readability.

switch (cond) {
   case 'one':
       // ...
       break;

   case 'two':
       // ...
       break;

    case 'three':
       // ...
       break;
}

I think it's more readable than

switch (cond) {
   case 'one':

       // ...

       break;

   case 'two':

       // ...

       break;

    case 'three':

       // ...

       break;
}

Other than that, there are warnings about $_GET, $_POST, etc. There must be some better way to deal with handling that data. Maybe in the next release. Any ideas?

Depending on what is the use of the data we can ignore the warning or filtering/validate the data.

For example, checking for the existence of a data like using isset($_GET['data']) doesn't make the code insecure so, the use of the // @codingStandardsIgnoreLine may be enough.

There are other at-comments. More info:
squizlabs/PHP_CodeSniffer#604
and
https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage

Other cases can be covered by using filter_var as first validation step.

For example:

$var = filter_var($_POST['key_data'], FILTER_SANITIZE_STRING');

if ( $var ) {
    // ....
}

I use filter_var and not filter_input because the first make the code more testable.

Then we can use any other function like: esc_url, esc_url_raw, sanitize_text_field, kses_post etc... depending on the context.

What are your thoughts about that?

[stevengliebe]

Thanks for the thoughts.

I'll make switch wpcs-compliant next release.

I opened an issue so $_POST, $_GET, etc. can be better handled in next release per your advice. #21