-
Notifications
You must be signed in to change notification settings - Fork 17
Added integration with the Events API endpoints #303
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
base: main
Are you sure you want to change the base?
Conversation
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.
Greptile Overview
Summary
This PR successfully integrates the WorkOS Events API by adding a comprehensive Events class, Event resource model, and thorough test coverage. The implementation follows security best practices with robust input validation and parameter sanitization.
Key Changes:
- Events.php: New API client class with
listEvents()andgetEventsByType()methods, comprehensive parameter validation, and event type whitelisting - Event.php: Resource model with getter methods, event type checking helpers (
isUserEvent(),isAuthenticationEvent(), etc.), and utility functions - EventsTest.php: Complete test suite covering happy paths, parameter validation, error handling, and resource functionality
- WorkOS.php: Minor formatting improvement (added blank line)
Security Features:
- Input validation and sanitization for all parameters
- Event type validation against hardcoded whitelist
- Parameter length limits to prevent abuse
- Proper exception handling with structured error messages
The implementation is well-structured, follows existing codebase patterns, and includes comprehensive error handling. All custom security rules are properly followed.
Confidence Score: 5/5
- This PR is safe to merge with no security or functionality concerns
- Score reflects thorough input validation, comprehensive test coverage, adherence to security best practices, and consistent code patterns. No logical errors, security vulnerabilities, or breaking changes identified.
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| lib/Events.php | 5/5 | New Events API integration class with robust input validation, event type whitelisting, and comprehensive parameter sanitization |
| lib/Resource/Event.php | 5/5 | Event resource model with getter methods, type checking helpers, and utility functions for working with event data |
| tests/WorkOS/EventsTest.php | 5/5 | Comprehensive test suite covering Events API functionality, parameter validation, error handling, and Event resource methods |
Sequence Diagram
sequenceDiagram
participant Client as PHP Application
participant Events as Events Class
participant Validation as Parameter Validation
participant API as WorkOS API
participant Resource as Event Resource
Client->>Events: listEvents($params)
Events->>Validation: validateAndSanitizeParams($params)
alt Invalid Parameters
Validation-->>Events: BadRequestException
Events-->>Client: Exception thrown
else Valid Parameters
Validation-->>Events: Sanitized parameters
alt Array events parameter
Events->>Events: implode(',', $params['events'])
end
Events->>API: Client::request('GET', 'events', null, $params)
API-->>Events: API Response (JSON)
Events-->>Client: Raw API response array
opt Creating Event Resources
Client->>Resource: Event::constructFromResponse($eventData)
Resource-->>Client: Event object with helper methods
end
end
Note over Client,Resource: Alternative flow: getEventsByType()
Client->>Events: getEventsByType($eventTypes, $params)
Events->>Events: validateEventTypes($eventTypes)
alt Invalid Event Types
Events-->>Client: BadRequestException
else Valid Event Types
Events->>Events: listEvents($params)
Events-->>Client: Filtered events response
end
4 files reviewed, no comments
nicknisi
left a comment
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.
Some quick changes/suggestions if you don't mind. I'm happy to help out as well. Also, please run composer format before finalizing to ensure consistent code style to appease CI. Thanks so much!
| * | ||
| * @return array<string> Array of valid event types | ||
| */ | ||
| public function getValidEventTypes() |
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'm a bit worried about the maintenance burden this creates. Whenever new events are added, they won't be available to use here until this list is updated and you update to that new version of the SDK.
The Node and Ruby SDKs skip this entirely and just pass strings to the API. I think that's valid but but not the
best DX. Instead, what if we keep the constants but remove the validation:
/**
* Common event types for IDE autocomplete.
* This list is NOT exhaustive - the API accepts additional values.
* @see https://workos.com/docs/events
*/
class EventTypes
{
public const USER_CREATED = 'user.created';
public const USER_UPDATED = 'user.updated';
// etc...
}Then in listEvents(), remove validateAndSanitizeParams() and just pass parameters to the API. Let the API be the source of truth for validation.
This gives PHP developers IDE autocomplete without blocking them from using new events before SDK updates. It also aligns with how Node, Ruby, and Go SDKs handle this.
Methods to remove:
validateEventTypes()validateAndSanitizeParams()getEventsByType()(just use listEvents() directly)
What are your thoughts?
| "object", | ||
| "event", | ||
| "data", | ||
| "created_at", |
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.
suggestion: I think this should be createdAt to match the mapped key in RESPONSE_TO_RESOURCE_KEY. Currently causes:
Warning: Undefined array key "created_at" in BaseWorkOSResource.php
"createdAt",
| "created_at", | |
| "createdAt", |
| // If no events parameter is provided, the API will return a 400 error | ||
| // Consider using getValidEventTypes() to get available event types | ||
|
|
||
| return Client::request(Client::METHOD_GET, $eventsPath, null, $params, true); |
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.
suggestion: The Events API only returns list_metadata.after (no before). To stay consistent with other list methods in this SDK (which all return [$before, $after,
$resources]), consider:
| return Client::request(Client::METHOD_GET, $eventsPath, null, $params, true); | |
| $response = Client::request(Client::METHOD_GET, $eventsPath, null, $params, true); | |
| $events = []; | |
| foreach ($response["data"] as $responseData) { | |
| \array_push($events, Resource\Event::constructFromResponse($responseData)); | |
| } | |
| $after = $response["list_metadata"]["after"] ?? null; | |
| return [null, $after, $events]; |
This gives users:
- Consistent return signature across all list methods
- Properly typed
Resource\Eventobjects instead of raw arrays - Clear indication that before isn't supported (always
null)
Description
Added integration with the Events API endpoints