xwp / stream

🗄️ Stream plugin for WordPress
https://wordpress.org/plugins/stream/
GNU General Public License v2.0
405 stars 119 forks source link

Refactored array_filter() to explicitly check !is_null() AND strlen() in class-log.php #1473

Closed justinmaurerdotdev closed 2 months ago

justinmaurerdotdev commented 6 months ago

… because passing 'strlen' as a callable caused warnings on null values.

Fixes #1349.

The previously-merged commits did not, in fact, fix this issue. Null values were being passed through array_filter() with 'strlen' as the callable, but strlen() no longer accepts null values. Should be fixed now, with an explicit null check added.

OLD:

$exclude_rules = array_filter($exclude, 'strlen' );

NEW:

$exclude_rules = array_filter( $exclude, function ( $val ) {
    return !is_null( $val ) && strlen( $val );
});

Checklist

Release Changelog

Release Checklist

Change [ ] to [x] to mark the items as done.

lkraav commented 5 months ago

Ouch, I wonder if strlen() changes also kill plans in #1312 :(

justinmaurerdotdev commented 5 months ago

Hmm, yeah. If it is being used to filter null values, then it likely will cause the same problem. The two refactoring options, as far as I can tell are:

  1. My above solution, or some similar variation where you check for null in the filter's callable, or
  2. An additional, array_filter() preceding the existing one to filter null values

I think Option 1 wins both for performance (Option 2 re-iterating the whole array) and presentation. There's no native is_not_null function. If there were, Option 2 would at least win on aesthetics:

$exclude_rules = array_filter($exclude, 'is_not_null' );
$exclude_rules = array_filter($exclude, 'strlen' );

In reality, it has to be something like

$exclude_rules = array_filter( $exclude, function ( $val ) {
    return !is_null( $val );
});
$exclude_rules = array_filter($exclude, 'strlen' );

So, we might as well combine them and save the extra loop. Though, this may be a sign of some upstream issue with how the how/why the null values are there in the first place... I'm new to this codebase, so I can't speak to that at all.

ocean90 commented 5 months ago

I already created a PR for this in #1466 which also ensures that only strings are passed to strlen().