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

Replace strlen with custom callback that accepts null #1513

Merged
merged 2 commits into from
Jul 22, 2024

Conversation

delawski
Copy link
Contributor

Fixes #1349
Supersedes #1466

This fixes a deprecation notice in PHP 8:

PHP Deprecated: strlen(): Passing null to parameter #1 ($string) of type string is deprecated in classes/class-log.php on line 217

It addressed the review feedback left on #1466 which pointed out that null values should be filtered out from the array.

In the end, keeping or removing null values in this particular scenario does not seem to matter. The $exclude_rules array is only passed to the record_matches_rules() method which ignores null values anyway:

if ( ! isset( $record[ $exclude_key ] ) || is_null( $exclude_value ) ) {

However, for the sake of keeping the business logic the same, I've opted for this approach instead of the one in #1466.

Checklist

  • 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.

This is done due to `strlen` not accepting `null` in PHP 8+.
@delawski delawski added the compatibility PHP or WordPress version compatibility related issue label Jul 22, 2024
@delawski delawski added this to the 4.0.1 milestone Jul 22, 2024
@delawski delawski self-assigned this Jul 22, 2024
Copy link
Contributor

@tharsheblows tharsheblows left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm approving but not merging -- mainly because I want to make sure I understand what's going on in the bit above it. Once you've had a look, you can merge!

$exclude_rules = array_filter(
$exclude,
function ( $value ) {
return ! is_null( $value ) && '' !== $value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@delawski I am approving this but I think all we need is a check on is_null()? I'm kind of asking because I want to make sure I understand the bit above it 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I absolutely love your reviews, @tharsheblows! You're totally right about the second check (removed in 9d47e25) - in this scenario it's redundant since we are explicitly setting every empty property to null above.

One of the solutions I was considering and trying out involved setting the properties above to empty strings instead of null so that the original strlen-based solution would still work. I guess I got lost somewhere in between 😅

@delawski delawski merged commit 46203c9 into develop Jul 22, 2024
2 checks passed
@delawski delawski deleted the fix/1349-strlen-compat branch July 22, 2024 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility PHP or WordPress version compatibility related issue
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
2 participants