Skip to content

Fix small bug in DirectorySync #291

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

Merged
merged 2 commits into from
Jun 25, 2025
Merged

Fix small bug in DirectorySync #291

merged 2 commits into from
Jun 25, 2025

Conversation

gcarvelli
Copy link
Contributor

@gcarvelli gcarvelli commented Jun 24, 2025

Description

Fixes a bug and a few typos in PHPDocs.

Documentation

Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.

[ ] Yes

If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.

@gcarvelli gcarvelli requested a review from a team as a code owner June 24, 2025 21:03
@gcarvelli gcarvelli requested a review from nicknisi June 24, 2025 21:03
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Fixed a critical bug in DirectorySync's listGroups method and corrected documentation typos in multiple files.

  • Fixed incorrect variable usage in lib/DirectorySync.php where $group was wrongly used instead of $user parameter, which would cause silent failures in group filtering
  • Corrected docstring typos in lib/UserManagement.php and lib/SSO.php where 'DDomain' was fixed to 'Domain' in parameter descriptions

3 files reviewed, 1 comment
Edit PR Review Bot Settings | Greptile

@@ -97,7 +97,7 @@ public function listGroups(
$params["directory"] = $directory;
}
if ($user) {
$params["user"] = $group;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

$group is undefined in this scope, this never could have worked.

Copy link
Contributor

@mattgd mattgd left a comment

Choose a reason for hiding this comment

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

Thanks to you and Claude for this 🙇

@gcarvelli gcarvelli merged commit fe4f23b into main Jun 25, 2025
5 checks passed
@gcarvelli gcarvelli deleted the gio/small-fixes branch June 25, 2025 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants