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

Do not remove user role value from roles property. #27

Merged
merged 3 commits into from Aug 30, 2018

Conversation

oscarssanchez
Copy link
Contributor

@oscarssanchez oscarssanchez commented Jul 30, 2018

Hi @allan23 , could you please review this PR which addresses #25 ?

It seems that by using array_shift() to retrieve the first element of the roles property we are taking away this value, so the property ends up empty and it causes the user to not have any roles

Thanks.

@nylen
Copy link

nylen commented Aug 24, 2018

Hi @oscarssanchez, I encountered the same issue. I've confirmed that this fixes it.

Is there a reason to use reset instead of $user->roles[0]? To me this is more straightforward code. Side effects from using reset here are unlikely but theoretically possible.

@oscarssanchez
Copy link
Contributor Author

oscarssanchez commented Aug 28, 2018

Hi @nylen ,

Thanks for helping us testing this one. The logic was to follow the array_shift approach as much as possible without being disruptive, since i don't know why it was used before.

However, it seems the reason it's used must be to just extract that value. If so, I think accessing the property the way you suggested is indeed more straightforward. I updated the PR.

Thanks again!

@@ -85,7 +85,7 @@ public function set_custom_variables() {
if ( function_exists( 'newrelic_set_user_attributes' ) ) {
if ( is_user_logged_in() ) {
$user = wp_get_current_user();
newrelic_set_user_attributes( $user->ID, '', array_shift( $user->roles ) );
newrelic_set_user_attributes( $user->ID, '', $user->roles[0] );
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we verify that the index of [0] actually exists? Just in case the roles are hosed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @allan23 , I updated the PR :)

@allan23 allan23 merged commit 62dec69 into 10up:master Aug 30, 2018
@nylen
Copy link

nylen commented Aug 30, 2018

Thanks @oscarssanchez and @allan23!

Will there be a release to the plugin directory to address this issue, or should we update the plug-in from this repo for now?

@allan23
Copy link
Contributor

allan23 commented Aug 30, 2018

@nylen this is live on wp.org. Be sure to upgrade to 1.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants