libreform / wp-libre-form

Easy native HTML5 forms for WordPress. Version 1.5 is unmaintained, but works without issue. 2.0 has been rewritten from the ground, and can be found at https://github.com/libreform/libreform
https://wordpress.org/plugins/wp-libre-form
GNU General Public License v3.0
67 stars 27 forks source link

Should we enable warnings in phpcs? #99

Closed k1sul1 closed 5 years ago

k1sul1 commented 6 years ago

Our current phpcs config ignores warnings. Warnings are bad, m'kay?

> ./vendor/bin/phpcs --standard=./phpcs.xml -p .
W...W..

FILE: ...wp-content/plugins/wp-libre-form/classes/class-cpt-wplf-form.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 10 WARNINGS AFFECTING 10 LINES
----------------------------------------------------------------------
 356 | WARNING | Line exceeds 120 characters; contains 128 characters
 361 | WARNING | Found: ==. Use strict comparisons (=== or !==).
 378 | WARNING | Line exceeds 120 characters; contains 267 characters
 381 | WARNING | Line exceeds 120 characters; contains 155 characters
 391 | WARNING | Line exceeds 120 characters; contains 156 characters
 401 | WARNING | Line exceeds 120 characters; contains 165 characters
 411 | WARNING | Line exceeds 120 characters; contains 155 characters
 423 | WARNING | Line exceeds 120 characters; contains 172 characters
 542 | WARNING | Line exceeds 120 characters; contains 131 characters
 549 | WARNING | Line exceeds 120 characters; contains 121 characters
----------------------------------------------------------------------

FILE: ...tdocs/wp-content/plugins/wp-libre-form/inc/wplf-form-actions.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 8 WARNINGS AFFECTING 8 LINES
----------------------------------------------------------------------
  10 | WARNING | [ ] Line exceeds 120 characters; contains 124
     |         |     characters
  17 | WARNING | [ ] Line exceeds 120 characters; contains 130
     |         |     characters
  19 | WARNING | [ ] Line exceeds 120 characters; contains 134
     |         |     characters
  21 | WARNING | [ ] Line exceeds 120 characters; contains 122
     |         |     characters
  50 | WARNING | [ ] Line exceeds 120 characters; contains 240
     |         |     characters
 122 | WARNING | [ ] Line exceeds 120 characters; contains 139
     |         |     characters
 125 | WARNING | [ ] Line exceeds 120 characters; contains 181
     |         |     characters
 144 | WARNING | [x] Usage of ELSE IF is discouraged; use ELSEIF
     |         |     instead
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 759ms; Memory: 14Mb

Nothing too serious right now, but they often show potential problems with the code, and we should treat warnings like we treat errors; fix them.

Extra long lines aren't usually serious, but they are practically unreadable. If an if block goes over the limit, it probably could be simplified by using variables, making the code more readable.

And == is just begging for trouble.

Simply remove -n from composer.json lint script to enable. But should we?

When warnings are useless, they can be swallowed by using a special comment syntax, as seen in #100.