-
Notifications
You must be signed in to change notification settings - Fork 651
Respect-show_avatars-comments-#2248
#2271
Respect-show_avatars-comments-#2248
#2271
Conversation
Possible fix for WP-API#2248. Respects the show_avatars option when retrieving avatars data for comments. Modeled after WP-API#2151.
Attempting to solve test failures.
| ), | ||
| ); | ||
|
|
||
| if ( get_option( 'show_avatars' ) ) { |
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.
This would be better off as a strict check (=== true) if we can.
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.
Okay, will fix. In the codex it is stored as 1 and 0 integer type. Is this changed to a boolean or should I check for 1 === get_option( 'show_avatars' )?
|
Looks pretty good 👍 Small change in the check, but otherwise seems pretty sensible. |
Added strict check for get_option( 'show_avatars' ) should it be a boolean? Attempt to fix testing.
Third attempt.
Fourth attempt
|
Hallelujah! |
| ), | ||
| ); | ||
|
|
||
| if ( '1' === get_option( 'show_avatars' ) ) { |
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 this should be a if ( get_option... the strict type check, while correct for most cases, is somewhat unintuitive, as this really should be a bool. +1 for allowing any truthy value.
|
Should we also apply the same to the users controller? The database option name is |
|
I'm 👎 on a strict check in this context. A filter from a plugin or theme should be able to return a truthy or falsy value, and see the API work as expected. |
|
@danielbachhuber So do you believe we should put it back to: if ( get_option( 'show_avatars' ) ) |
|
I think we should use |
|
I'm OK with the truthy/loose check so long as it doesn't cause side effects (i.e. we don't accidentally catch |
Core uses I think we're fine using |
|
Okay, I will change it back give me a second. |
…ct-`show_avatars`-comments#2248
Removing strict check get_option( 'show_avatars' ).
|
Build fails because of #1488 (comment) Merging because curies need to be fixed in this release regardless. |
…ments#2248 Respect-`show_avatars`-comments-#2248
Possible fix for #2248. Respects the show_avatars option when
retrieving avatars data for comments. Modeled after #2151.