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

gppa-ignore-current-user-filter-for-admins.php: Fixed a potential PHP error. #728

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

saifsultanc
Copy link
Contributor

Context

⛑️ Ticket(s): https://secure.helpscout.net/conversation/2412200474/56932?folderId=3808239

Summary

The snippet causes a fatal error when used on-site to remove the current user ID filter for admins.

@saifsultanc saifsultanc added the bug Something isn't working label Nov 9, 2023
@@ -25,7 +25,7 @@
return $query_builder_args;
}

array_pop( $query_builder_args['where'][ $filter_group_index ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on this usage, this is wiping out the entire where and effectively removing all filters from what I can see.

My guess is that the issue has to do with the filter using special_value:current_user:ID not being the last filter in the group.

You'll need to loop through $query_builder_args['where'][ $filter_group_index ] and find the filter that has $filter['value'] === 'special_value:current_user:ID' and remove it from the $query_builder_args['where'][ $filter_group_index ] array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Actually $filter_group_index does that job fine. What is happening specifically in this test scenario is that there is only 1 filter and that is using special_value:current_user:ID. So unsetting messes up the overall structure of the $query_builder_args which is what I've fixed in the next test and pushed it. @claygriffiths

Comment on lines 29 to 32
if ( count( $query_builder_args['where'][ $filter_group_index ] ) == 0 ) {
$query_builder_args['where'] = array();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What if there are multiple groups?

For example, if $filter_group_index is 1 here, and we have another on 2, this would clear that out as well.

@claygriffiths claygriffiths force-pushed the saif/fix/56932-fix-filter-warning branch from 1606381 to 2d269e2 Compare November 16, 2023 18:39
@claygriffiths claygriffiths marked this pull request as ready for review November 16, 2023 18:39
Copy link

Warnings
⚠️ When ready, don't forget to request reviews on this pull request from your fellow wizards.

Generated by 🚫 dangerJS against 2d269e2

@claygriffiths claygriffiths changed the title gppa-ignore-current-user-filter-for-admins.php: Fixed an issue with snippet causing a fatal error. gppa-ignore-current-user-filter-for-admins.php: Fixed a potential PHP error. Nov 16, 2023
@claygriffiths claygriffiths merged commit 8b8b385 into master Nov 16, 2023
3 checks passed
@claygriffiths claygriffiths deleted the saif/fix/56932-fix-filter-warning branch November 16, 2023 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

2 participants