#27882 closed defect (bug) (fixed)
Address some issues found running Scrutinizer
Reported by: | wonderboymusic | Owned by: | |
---|---|---|---|
Milestone: | 4.0 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | needs-patch |
Focuses: | Cc: |
Description
Scrutinizer is great: https://scrutinizer-ci.com/tour/measure-and-improve-code-quality
It found over 3000 problems with our code, we could perhaps heed its advice on one or two.
Attachments (3)
Change History (94)
#5
@
11 years ago
- Keywords close added
Marko is right, I've also been tracking Scrutinizer recently too, but until we have specific fixes in mind (and ideally patches to go with them), it's pointless to open a general "placeholder" ticket that can never be discussed (because it's not about specific issues), resolved, and closed. There will always be new Scrutinizer issues, and very likely several that will never be fixed for numerous reasons mostly related to back-compat.
FWIW, here's my public Scrutinizer report since none was even linked yet:
https://scrutinizer-ci.com/g/tierra/wordpress/issues/master
Please note that Scrutinizer has *not* been configured for known WP-specific rulesets, so much of these reports are going to be incorrect or not applicable, especially for 3rd party libs.
#6
@
11 years ago
- Milestone Future Release deleted
- Resolution set to invalid
- Status changed from new to closed
Only closing because I have no interest in this minutiae.
#7
follow-up:
↓ 8
@
11 years ago
Some of it seems pretty valid, though, e.g.:
"The variable $url seems only to be defined at a later point. Did you maybe move this code here without moving the variable definition?"
#8
in reply to:
↑ 7
@
11 years ago
Replying to Denis-de-Bernardy:
"The variable $url seems only to be defined at a later point. Did you maybe move this code here without moving the variable definition?"
It's defined by wp_reset_vars()
: tags/3.9/src/wp-admin/includes/misc.php#L263.
#10
@
11 years ago
wonderboymusic was well within his rights and responsibilities (as a core committer, no less) to open up a tracking ticket for investigation. If you have nothing valuable to say, don't say it.
#11
@
11 years ago
- Keywords needs-patch added; close removed
- Milestone set to 4.0
- Resolution invalid deleted
- Status changed from closed to reopened
#69
@
10 years ago
After all of that Hack/HHVM cleanup, Scrutinizer now finds 500 less issues with the code. ~2700 down from ~3300.
#77
@
10 years ago
- Resolution set to fixed
- Status changed from reopened to closed
Gonna mark this as fixed after 64 healthy commits. Almost all of the remaining issues are related to:
- Top-level code, global variables. There a lot of variables that get set in a file and are assumed by us to spill down to other places (think, the admin). We should get away from top-level code, but we have tons of it.
- Parameters to functions/methods that are unused but will remain for BC.
- Parameters to methods that only exist in subclasses to match parent methods' signatures.
- All kinds of garbage in the files we include from external libraries.
- Some actual issues we can address in other ways outside of this ticket.
#78
@
10 years ago
I have an outstanding patch in #26604 for dashboard.php
, to remove 6 lines of confusing dead code. I *also* speculated about deprecating unused dashboard functions and options, but the patch itself removes code that does nothing at all. Ping me there if you want a fresh patch.
#79
@
10 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
There are a few new issues
#82
in reply to:
↑ 80
;
follow-up:
↓ 85
@
10 years ago
Replying to wonderboymusic:
Adds a unit test to demonstrate that the order of
case
anddefault
in aswitch
statement does not matter.
The order may not matter but makes it (much) harder to read/follow when default
is not at the bottom. Same for the unreachable break;
after return
and die()
. As far as I remember these were added for readability as switch() is one of the "harder to read" constructs, especially for inexperienced users. (Omitting a break
merges the cases, some people miss or misunderstand that).
I don't particularly mind either way, but maybe we need to add some rules for switch()
to the coding standards. Few years ago there were some.
Not sure we need a "demonstration test". Also it is not complete: it doesn't test the default case. A better demo would be something like:
function _switch_order_helper( $var ) { switch ( $var ) { default: $return = 'default'; break; case 1: $return = 'case 1'; break; } return $return; } function test_switch_order() { $this->assertEquals( 'case 1', _switch_order_helper( 1 ) ); $this->assertEquals( 'default', _switch_order_helper( 2 ) ); }
#83
follow-up:
↓ 84
@
10 years ago
The "demonstration tests" have been added to test assumptions in basic.php
. We've never been burned by having too many unit tests, that's for sure.
#84
in reply to:
↑ 83
@
10 years ago
Replying to wonderboymusic:
Right, but for whom does it "demonstrate"? I doubt it the people that need this demonstration would actually look for it in the tests. Most likely they would go to php.net.
#85
in reply to:
↑ 82
@
10 years ago
Replying to azaozz:
The order may not matter but makes it (much) harder to read/follow when
default
is not at the bottom. Same for the unreachablebreak;
afterreturn
anddie()
. As far as I remember these were added for readability as switch() is one of the "harder to read" constructs, especially for inexperienced users. (Omitting abreak
merges the cases, some people miss or misunderstand that).
I don't particularly mind either way, but maybe we need to add some rules for
switch()
to the coding standards. Few years ago there were some.
Maybe styling them something like this would improve readability?
switch ( $var ) { case 1: /* ... */ break; case 2: /* ... */ return; case 3: /* ... */ // fallthru case 4: /* ... */ exit; default: /* ... */ }
I think adding a comment whenever one case
falls through into another is especially useful. But, you know, everybody has their own style.
#86
@
10 years ago
The only 2 things I have an opinion on:
- can we standardize on
default
being at the bottom? - Automation and static analysis are helpful ways for us to examine our whole code base. Every single tool complains about
break
afterreturn|exit|die
, whatever. We refactorswitch
s almost never. Is our process for code review so broken that a futurecase
is guaranteed to carelessly fall through unless we proactively add extraneousbreak
s? We reopen tickets milliseconds after documentation changes have ambiguous grammatical errors. I may be outvoted on this one, but I think this is the least of our concerns - conversely, that same argument could have been made for me not to remove them in the first place. Sure, but I would rather us be able take advantage of more CI tools now and the future which is better for everyone.
#88
@
10 years ago
The only 2 things I have an opinion on:
- can we standardize on default being at the bottom?
Thinking we should. Much easier to read.
Sure, but I would rather us be able take advantage of more CI tools now and the future which is better for everyone.
Agreed. Not having unreachable break;
is "the right way to go". We probably should require a comment when the break is missing on purpose though, as suggested by @jdgrimes above. // no break
, // fall through
, anything...
This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.
10 years ago
#91
@
10 years ago
In wp-admin/includes/class-wp-upgrader.php there are some undefined properties http://i.imgur.com/Lxipd0Y.png
Shouldn't we create tickets based on the advice instead of creating an unclear focussed ticket?