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

Fix more PHP 8.1 compatibility issues #1494

Merged
merged 19 commits into from
Jul 22, 2024
Merged

Fix more PHP 8.1 compatibility issues #1494

merged 19 commits into from
Jul 22, 2024

Conversation

delawski
Copy link
Contributor

@delawski delawski commented Jul 12, 2024

Fixes #1493, #1492, #1442.

Describe your approach and how it fixes the issue.

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.

Release Changelog

  • Fix: Use PHP DateTimeImmutable for handling dates instead of relying on Carbon.
  • Fix: PHP 8.1 compatibility warnings and errors.

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.

@delawski delawski added this to the 4.0.1 milestone Jul 12, 2024
Copy link
Member

@bartoszgadomski bartoszgadomski left a comment

Choose a reason for hiding this comment

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

Looks good to me! Added few comments below, please take a look:

alerts/class-alert-type-email.php Outdated Show resolved Hide resolved
Comment on lines 232 to 237
$user_value = ! empty( $user->$user_field ) ? $user->$user_field : $user->user_login;
$user_value = '';
if ( ! empty( $user->$user_field ) ) {
$user_value = $user->$user_field;
} elseif ( ! empty( $user->user_login ) ) {
$user_value = $user->user_login;
}
Copy link
Member

Choose a reason for hiding this comment

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

@delawski Is there any difference in how the code resolves? Single line with ternary operator is more readable, in my opinion.

Is the case here that the user_field or user_login properties might not exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, my understanding is that $user may sometimes be undefined. There was a bug report in which an event triggered by a cron task had no $user and this yielded a PHP warning.

So in this case, on one hand the $user_field is a filterable variable which we don't know if it exists, and on another hand the $user itself may be undefined.

I didn't want to wrap that entire snippet in a conditional and also didn't want to have a nested tertiary operator. That's why I went with, I guess, a rather simple and straight-forward conditionals.

Am I missing something? I'm happy to rewrite it in order to make it simpler if possible.

Copy link
Member

Choose a reason for hiding this comment

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

@delawski The problem here is the get_user_by function call on line 220: this function can return false on failure.

I'd suggest to wrap the $user_value check in a condition like this:

$user       = get_user_by( 'id', $user_id );
$user_value = '';

if ( $user instanceof WP_User ) {
	/**
	 * Filter User data field
	 *
	 * Defaults to 'user_login'.
	 *
	 * @param object $alert The Alert.
	 * @param array  $recordarray The Record's data.
	 * @return string
	 */
	$user_field = apply_filters( 'wp_stream_alert_ifttt_user_data_value', 'user_login', $alert, $recordarr );
	$user_value = ! empty( $user->$user_field ) ? $user->$user_field : $user->user_login;
}

In other words: if $user is false (and that is a valid response of the get_user_by function), the $user_value will always be empty, no matter what's returned in a filter; so we could skip that logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes total sense! I've used your suggestion in e943b98. Note that I'm also checking if $recordarr['user_id'] even exists (I'm not familiar yet with how $recordarr structure may look like for various connectors).

classes/class-date-interval.php Show resolved Hide resolved
classes/class-date-interval.php Outdated Show resolved Hide resolved
connectors/class-connector-acf.php Outdated Show resolved Hide resolved
classes/class-date-interval.php Show resolved Hide resolved
delawski and others added 3 commits July 15, 2024 17:02
Co-authored-by: Bartosz Gadomski <kontakt@bartoszgadomski.pl>
Co-authored-by: Bartosz Gadomski <kontakt@bartoszgadomski.pl>
@delawski delawski added the compatibility PHP or WordPress version compatibility related issue label Jul 16, 2024
@delawski delawski self-assigned this Jul 17, 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.

There are two random thoughts as comments on this. It's approved though, they don't need to be done!

classes/class-date-interval.php Outdated Show resolved Hide resolved
$user = new \WP_User( $user_id );
$user_email = ! empty( $user->user_email ) ? $user->user_email : '';
$user_login = ! empty( $user->user_login ) ? $user->user_login : '';
$user_display_name = ! empty( $user->display_name ) ? $user->display_name : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

❓question
Could a user without a display name be something involving the $user_id here? Like 'Unknown user, user_id: 21'

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'm not sure if I follow. Can you elaborate on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@delawski If there's no user, like the user has been deleted, then it'd be nice to have the display name be shown as something like "Unknown user {$user_id}"? I think we did this somewhere else too, it's the same sort of thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I get it, thank you for explaining :) I've addressed it in 080e85f.

__( '%1$s\'s account %2$s Jetpack', 'stream' ),
$user->display_name,
esc_html( $user_display_name ),
Copy link
Contributor Author

@delawski delawski Jul 19, 2024

Choose a reason for hiding this comment

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

📝 note: Actually, I'm a bit confused if this should or should not be escaped here. Other strings (unchanged by this PR) are sometimes escaped and sometimes not escaped 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it looks like all record fields, including the message/summary, go through a sanitizer before they are stored in the DB. All HTML tags are stripped by the sanitizer:

stream/classes/class-db.php

Lines 118 to 128 in 275ed1d

// Sanitize all record values.
return array_map(
function ( $value ) {
if ( ! is_array( $value ) ) {
return wp_strip_all_tags( $value );
}
return $value;
},
$record
);

Since we don't expect any HTML tags in the user display name (and we definitely don't want any tag to be displayed there), we won't escape HTML letting wp_strip_all_tags do its job.

But I guess in some cases we actually want to escape HTML so that tags are not stripped out and instead presented in the logs for better understanding of what has happened.

@delawski
Copy link
Contributor Author

@tharsheblows I pushed one more fix to a bug I discovered just today which is the same sort of an issue. It's here: 269ca93

When creating new Alerts, AJAX call is done to WP with a post_id = 'new' (when editing existing alerts, those are real IDs for the alert objects). Since the value was not empty, it passed through the logic up until it attempted to get e.g. ID or post_title out of the nonexistent post.

@delawski delawski merged commit 159e61f into develop Jul 22, 2024
2 checks passed
@delawski delawski deleted the fix/php-compatibility branch July 22, 2024 08:25
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
3 participants