Skip to content
This repository was archived by the owner on Sep 24, 2018. It is now read-only.

Conversation

@BE-Webdesign
Copy link
Member

Possible fix for #2248. Respects the show_avatars option when
retrieving avatars data for comments. Modeled after #2151.

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' ) ) {
Copy link
Member

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.

Copy link
Member Author

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' )?

@rmccue
Copy link
Member

rmccue commented Feb 12, 2016

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
@BE-Webdesign
Copy link
Member Author

Hallelujah!

),
);

if ( '1' === get_option( 'show_avatars' ) ) {
Copy link
Member

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.

@joehoyle
Copy link
Member

Should we also apply the same to the users controller? The database option name is show_avatars which makes it seem like it's not comment specific, though I understand that's where the option is in the dashbaord?

@joehoyle joehoyle added this to the 2.0 Beta 13 milestone Feb 13, 2016
@BE-Webdesign
Copy link
Member Author

@joehoyle I believe this was already done in users #2151. @rmccue suggested the strict check so if == is the way over === we can decide that in the meeting maybe?

@danielbachhuber
Copy link
Member

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.

@BE-Webdesign
Copy link
Member Author

@danielbachhuber So do you believe we should put it back to:

if ( get_option( 'show_avatars' ) )
or use
if ( true == get_option( 'show_avatars' ) )
or something else entirely?

@danielbachhuber
Copy link
Member

I think we should use if ( get_option( 'show_avatars' ) )

@rmccue
Copy link
Member

rmccue commented Feb 15, 2016

I'm OK with the truthy/loose check so long as it doesn't cause side effects (i.e. we don't accidentally catch 'n' or something).

@danielbachhuber
Copy link
Member

I'm OK with the truthy/loose check so long as it doesn't cause side effects

Core uses if ( ! get_option( 'show_avatars' ) ): https://core.trac.wordpress.org/browser/trunk/src/wp-includes/pluggable.php#L2356

I think we're fine using if ( get_option( 'show_avatars' ) ). @BE-Webdesign if you want to make that final change, we can get this landed.

@BE-Webdesign
Copy link
Member Author

Okay, I will change it back give me a second.

Removing strict check get_option( 'show_avatars' ).
@danielbachhuber
Copy link
Member

Build fails because of #1488 (comment)

Merging because curies need to be fixed in this release regardless.

danielbachhuber added a commit that referenced this pull request Feb 16, 2016
…ments#2248

Respect-`show_avatars`-comments-#2248
@danielbachhuber danielbachhuber merged commit 4787ce2 into WP-API:develop Feb 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants