-
-
Notifications
You must be signed in to change notification settings - Fork 254
Repair webhooks once again #1815
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
Conversation
📝 WalkthroughWalkthroughThe PR changes webhook processing to conditionally decode/wrap payload data and to build headers only for Regular webhook types. It also updates WebhookConfiguration::replaceVars to accept arrays or objects, converting objects to arrays before performing replacements. Changes
Sequence DiagramsequenceDiagram
participant Processor as ProcessWebhook
participant Config as WebhookConfiguration
participant Handler as Webhook Handler
Processor->>Processor: Check webhook type
alt Regular webhook
Processor->>Processor: If data is string → json_decode & wrap
Processor->>Processor: Build headers & perform replacements
else Other webhook types
Processor->>Processor: Leave data as-is
Processor->>Processor: Set empty headers
end
Processor->>Config: replaceVars(replacement, subject)
alt replacement is object
Config->>Config: Convert object → array via toArray()
else replacement is array
Config->>Config: Use array as-is
end
Config->>Config: Execute preg_replace_callback
Config-->>Processor: Return processed subject
Processor->>Handler: Send processed payload & headers
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/Jobs/ProcessWebhook.php (1)
32-35: Consider simplifying the data extraction logic.The multi-step extraction process (get first element, check count, reset if single) is difficult to follow. The purpose of this transformation isn't immediately clear from the code.
Consider adding a comment explaining why this transformation is necessary, or refactor to a dedicated method with a clear name like
extractWebhookData().app/Models/WebhookConfiguration.php (1)
176-180: Add defensive check for thetoArray()method.While the change to accept
array|objectadds flexibility, callingtoArray()without verifying the method exists could cause a fatal error if an incompatible object is passed.Consider adding a defensive check:
public function replaceVars(array|object $replacement, string $subject): string { if (is_object($replacement)) { + if (!method_exists($replacement, 'toArray')) { + $replacement = (array) $replacement; + } else { $replacement = $replacement->toArray(); + } } return preg_replace_callback(Alternatively, if only specific object types are expected (e.g., Eloquent models), consider using a more specific union type or interface constraint in the method signature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Jobs/ProcessWebhook.php(2 hunks)app/Models/WebhookConfiguration.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/Jobs/ProcessWebhook.php (1)
app/Models/WebhookConfiguration.php (1)
replaceVars(176-191)
🔇 Additional comments (2)
app/Jobs/ProcessWebhook.php (2)
37-39: Good defensive coding for type handling.The conditional check ensures JSON decoding only happens when necessary, and the fallback to an empty array prevents errors from invalid JSON.
60-68: Empty headers for non-Regular webhooks are correct
Discord webhooks don’t support custom headers, which matches the UI configuration.
Co-authored-by: MartinOscar <40749467+rmartinoscar@users.noreply.github.com>
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.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Jobs/ProcessWebhook.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/Jobs/ProcessWebhook.php (1)
app/Models/WebhookConfiguration.php (1)
replaceVars(176-191)
Something made webhooks be arrays now, this pull request should fix the problem.