Improve code quality

[GaryJones]

Fix some PHPCS and Scrutinizer issues.

[jrfnl]

Better:

if ( is_array( $this->plugins ) && array() !== $this->plugins )

[GaryJones]

Why is array() !== $this->plugins better than ! empty( $this->plugins )?

[jrfnl]

Where do I start ? Empty is hardly ever the correct function to use.

1. empty does not test for type while you need an array two lines later for the foreach which will throw a notice if it doesn't get one
2. empty will pass on a non-empty string, boolean true, a non-zero integer and float etc
3. empty actually doesn't pass on the string '0' (not relevant in this case, but confuses people often)
   etc...

Always test specifically for what you want to know - here: do we have a non-empty array. And in this case neither if ( $this->plugins ) nor if ( ! empty( $this->plugins ) ) does.

See: http://phpcheatsheets.com/test/

[GaryJones]

1. and 2. - I'm not asking why the need for is_array(), I'm asking why you're checking against exactly array() instead of using empty(). By the time we get to that check, we've already established we're looking at it being an array, so 2. is irrelevant.

Performance wise, ! empty() seems to be quicker than !== array().

! empty() is also what Scrutinizer recommended so I would expect they've done their homework.

! empty() is also slightly more self-documenting than !== array().

I'm happy to add the is_array() check, but the ! empty() should be used over !== array() IMO i.e. if ( is_array( $this->plugins ) && ! empty( $this->plugins ) )

[jrfnl]

As long as you first check for is_array(), it doesn't matter whether you use $var !== array(), ! empty( $var ) or count( $var ) > 0 for the result.

Using count() is definitely slowest, so either of the first two is better. I seemed to remember that !==array() was slightly faster (might depend on the PHP version), but am happy to accept empty() just as well.

[GaryJones]

There's no technical reason for is_array() being first, and according to the bottom of http://www.phpbench.com/, is_array() is relatively expensive to check compared to seeing if it's some type that is empty.

So: "Does this variable have a non-nil value, and is it an array?" may be quicker to abort for bad values compared to "Is this variable an array, and does it have a non-nil value?"

Of course, this is a micro-optimisation, so I'm happy with the already committed order.

[jrfnl]

As you say, micro-optimization as both will normally need to pass anyway.

[GaryJones]

Changed line 873/4 to be a cast to an array, since array_filter() was being called on it before checking it was not empty.

[shivapoudel]

Why not defined( 'WP_DEBUG' ) ?

[GaryJones]

Because I could have define( 'WP_DEBUG', false ).

WP already defines it as false if we've not defined it as something else, so the WP_DEBUG constant will always be defined - we just want to check it's been defined as true.