Skip to content

Conversation

@blobaugh
Copy link
Contributor

Description

Added integration with the Events API endpoints

@blobaugh blobaugh requested a review from a team as a code owner September 23, 2025 19:05
@blobaugh blobaugh requested a review from dandorman September 23, 2025 19:05
Copy link
Contributor

@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.

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() and getEventsByType() 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
Loading

4 files reviewed, no comments

Edit Code Review Bot Settings | Greptile

Copy link
Member

@nicknisi nicknisi left a 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()
Copy link
Member

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",
Copy link
Member

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",
Suggested change
"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);
Copy link
Member

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:

Suggested change
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\Event objects instead of raw arrays
  • Clear indication that before isn't supported (always null)

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