Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

justinmaurerdotdev
Copy link

… 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

  • [n/a] Project documentation has been updated to reflect the changes in this pull request, if applicable.
  • I have tested the changes in the local development environment (see contributing.md).
  • I have added phpunit tests.

Release Changelog

  • Fix: Describe a bug fix included in this release.
  • New: Describe a new feature in this release.

Release Checklist

  • This pull request is to the master branch.
  • Release version follows semantic versioning. Does it include breaking changes?
  • Update changelog in readme.txt.
  • Bump version in stream.php.
  • Bump Stable tag in readme.txt.
  • Bump version in classes/class-plugin.php.
  • Draft a release on GitHub.

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

Justin Maurer added 2 commits January 10, 2024 11:06
… because passing 'strlen' as a callable caused warnings on null values.
@lkraav
Copy link
Contributor

lkraav commented Jan 10, 2024

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

@justinmaurerdotdev
Copy link
Author

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
Copy link
Contributor

ocean90 commented Jan 15, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PHP 8.1: PHP Deprecated: strlen(): Passing null to parameter #1 ($string) of type string is deprecated
3 participants