-
Notifications
You must be signed in to change notification settings - Fork 904
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
Avoid storing the full user object in notifications meta #20800
Changes from 20 commits
4377168
56eb126
32f466f
33e9cf7
35ad0b5
671c37a
40cf598
d7c2c2e
3d60c25
b9ffccc
a322c93
426c143
494da26
d7c62c3
b098118
ae6aec8
c73a8b3
af9d09e
f06f1ee
2fad611
0015edd
f3b7c7f
b0427ae
8a63a68
1567063
2fa9853
6220e18
7115eec
2bed3d5
0ecd600
607d8ad
65f3241
c3d7f1e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,7 +72,7 @@ class Yoast_Notification { | |
private $defaults = [ | ||
'type' => self::UPDATED, | ||
'id' => '', | ||
'user' => null, | ||
'user_id' => null, | ||
'nonce' => null, | ||
'priority' => 0.5, | ||
'data_json' => [], | ||
|
@@ -110,12 +110,12 @@ public function get_id() { | |
} | ||
|
||
/** | ||
* Retrieve the user to show the notification for. | ||
* Retrieve the id of the user to show the notification for. | ||
* | ||
* @return WP_User The user to show this notification for. | ||
* @return int The user id. | ||
*/ | ||
public function get_user() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing the return value and type is a BC break. I think this is actually just a helper for the method below. Probably we can just deprecate it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I deprecated it, it wouldn't make sense anymore after this PR gets merged 🙂 |
||
return $this->options['user']; | ||
return $this->options['user_id']; | ||
} | ||
|
||
/** | ||
|
@@ -127,7 +127,7 @@ public function get_user() { | |
*/ | ||
public function get_user_id() { | ||
if ( $this->get_user() !== null ) { | ||
return $this->get_user()->ID; | ||
return $this->get_user(); | ||
} | ||
return get_current_user_id(); | ||
} | ||
|
@@ -220,7 +220,7 @@ public function display_for_current_user() { | |
*/ | ||
public function match_capabilities() { | ||
// Super Admin can do anything. | ||
if ( is_multisite() && is_super_admin( $this->options['user']->ID ) ) { | ||
if ( is_multisite() && is_super_admin( $this->options['user_id'] ) ) { | ||
return true; | ||
} | ||
|
||
|
@@ -280,7 +280,15 @@ public function match_capabilities() { | |
* @return bool | ||
*/ | ||
private function has_capability( $capability ) { | ||
$user = $this->options['user']; | ||
$user_id = $this->options['user_id']; | ||
if ( ! is_numeric( $user_id ) ) { | ||
return false; | ||
} | ||
$user = get_user_by( 'id', $user_id ); | ||
if ( ! $user ) { | ||
return false; | ||
} | ||
|
||
return $user->has_cap( $capability ); | ||
} | ||
|
||
|
@@ -396,9 +404,10 @@ private function normalize_options( $options ) { | |
$options['capabilities'] = [ 'wpseo_manage_options' ]; | ||
} | ||
|
||
// Set to the current user if not supplied. | ||
if ( $options['user'] === null ) { | ||
$options['user'] = wp_get_current_user(); | ||
// Set to the id of the current user if not supplied. | ||
if ( $options['user_id'] === null ) { | ||
$user = wp_get_current_user(); | ||
$options['user_id'] = (int) $user->ID; | ||
pls78 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
return $options; | ||
|
@@ -413,4 +422,22 @@ private function normalize_options( $options ) { | |
private function parse_attributes( &$value, $key ) { | ||
$value = sprintf( '%s="%s"', sanitize_key( $key ), esc_attr( $value ) ); | ||
} | ||
|
||
/** | ||
* Unsets user field if present and set user_id. | ||
* | ||
* @internal This is meant to be used by the Yoast SEO plugin only. | ||
* | ||
* @return bool If the user field was present. | ||
*/ | ||
public function user_to_user_id() { | ||
if ( array_key_exists( 'user', $this->options ) ) { | ||
// No check needed as we call this once the notification has already been stored. | ||
$user_id = $this->options['user']->ID; | ||
unset( $this->options['user'] ); | ||
$this->options['user_id'] = $user_id; | ||
pls78 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return true; | ||
} | ||
return false; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -806,15 +806,15 @@ public function test_add_notifications_for_multiple_users() { | |
$notification_for_user_1 = new Yoast_Notification( | ||
'Hello, user 1!', | ||
[ | ||
'user' => $user_mock_1, | ||
'user_id' => $user_mock_1->ID, | ||
'capabilities' => [ 'wpseo_manage_options' ], | ||
] | ||
); | ||
|
||
$notification_for_user_2 = new Yoast_Notification( | ||
'Hello, user 2!', | ||
[ | ||
'user' => $user_mock_2, | ||
'user_id' => $user_mock_2->ID, | ||
'capabilities' => [ 'wpseo_manage_options' ], | ||
] | ||
); | ||
|
@@ -841,13 +841,15 @@ public function test_add_notifications_only_once_for_user() { | |
|
||
$instance = $this->get_notification_center(); | ||
|
||
$user_mock = $this->mock_wp_user( 3, [ 'wpseo_manage_options' => true ] ); | ||
$user_id = $this->factory->user->create(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there is a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done 👍 |
||
$user = new WP_User( $user_id ); | ||
$user->add_cap( 'wpseo_manage_options' ); | ||
|
||
$notification = new Yoast_Notification( | ||
'Hello, user 3!', | ||
[ | ||
'id' => 'Yoast_Notification_Test', | ||
'user' => $user_mock, | ||
'user_id' => $user_id, | ||
'capabilities' => [ 'wpseo_manage_options' ], | ||
] | ||
); | ||
|
@@ -856,7 +858,7 @@ public function test_add_notifications_only_once_for_user() { | |
$instance->add_notification( $notification ); | ||
|
||
$expected = [ $notification ]; | ||
$actual = $instance->get_notifications_for_user( 3 ); | ||
$actual = $instance->get_notifications_for_user( $user_id ); | ||
|
||
$this->assertEquals( $expected, $actual ); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I detected an order problem. In
filter_notification_current_user
theuser_id
is already expected to be there.But here you convert it only after the filter.
I was thinking of a solution:
array_to_notification
methodBut then I thought: why not check in the constructor of the notification? Specifically the
normalize_options
already has some logic that looks like a spot at first sight?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I reached a good middle point:
array_to_notification
methodarray_map
callingarray_to notification
Yoast_Notification
🙂By doing so we can still set the
notifications_need_storage
flag and save to db the new user_id asap