Skip to content

Feat: Add OneSignal push provider adapter (#7726)#131

Open
deepshekhardas wants to merge 2 commits into
utopia-php:mainfrom
deepshekhardas:feat/7726-onesignal-adapter
Open

Feat: Add OneSignal push provider adapter (#7726)#131
deepshekhardas wants to merge 2 commits into
utopia-php:mainfrom
deepshekhardas:feat/7726-onesignal-adapter

Conversation

@deepshekhardas

Copy link
Copy Markdown

No description provided.

@greptile-apps

greptile-apps Bot commented Jun 17, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds a new OneSignal push provider adapter for utopia-php/messaging, implementing the standard Push adapter interface with support for titles, bodies, data payloads, actions, images, sounds, badges, icons, colors, tags, content availability, and priority.

  • The adapter builds a OneSignal v1 notification payload and maps most Push message fields, with parent::__construct() correctly called and priority correctly mapped via a match expression.
  • The implementation targets the legacy https://onesignal.com/api/v1/notifications endpoint using include_player_ids and Authorization: Basic — all three are deprecated by OneSignal in favor of api.onesignal.com/notifications, include_subscription_ids, and Authorization: Key, respectively.
  • Several payload-mapping and response-handling issues flagged in earlier review rounds remain unresolved in this revision.

Confidence Score: 2/5

Not ready to merge — multiple correctness issues in the payload-building and response-handling logic remain unaddressed from prior review rounds.

The adapter has several open correctness gaps: content_available is hardcoded to true even when the caller passes false, the iOS sound name has .wav unconditionally appended (mangling values like default), HTTP 200 responses are treated as full-delivery success even when OneSignal returns per-recipient errors in the body, and the critical alert flag is silently dropped. These issues affect real notification delivery behavior across the most common push use-cases the shared test suite exercises.

src/Utopia/Messaging/Adapter/Push/OneSignal.php — the payload construction and response parsing logic need the most attention before this is production-ready.

Important Files Changed

Filename Overview
src/Utopia/Messaging/Adapter/Push/OneSignal.php New OneSignal push adapter that targets the legacy v1 API endpoint with deprecated include_player_ids field; several correctness issues remain in the payload-building logic and response-handling path.
tests/Messaging/Adapter/Push/OneSignalTest.php Minimal integration test that inherits all shared push test cases from Base; consistent with the FCM and APNS test pattern in the repository.

Reviews (3): Last reviewed commit: "fix: add parent constructor, fix deliver..." | Re-trigger Greptile

Comment thread src/Utopia/Messaging/Adapter/Push/OneSignal.php
Comment thread src/Utopia/Messaging/Adapter/Push/OneSignal.php
Comment thread src/Utopia/Messaging/Adapter/Push/OneSignal.php
Comment thread src/Utopia/Messaging/Adapter/Push/OneSignal.php
Comment on lines +65 to +68
if (!\is_null($message->getSound())) {
$payload['android_sound'] = $message->getSound();
$payload['ios_sound'] = $message->getSound() . '.wav';
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Do not rewrite sounds

Other push adapters pass the message sound through unchanged, and callers may pass values like default or an already-qualified filename. Appending .wav here changes default to default.wav and chime.wav to chime.wav.wav, which can make iOS use the wrong sound or no custom sound at all.

Suggested change
if (!\is_null($message->getSound())) {
$payload['android_sound'] = $message->getSound();
$payload['ios_sound'] = $message->getSound() . '.wav';
}
if (!\is_null($message->getSound())) {
$payload['android_sound'] = $message->getSound();
$payload['ios_sound'] = $message->getSound();
}

Comment thread src/Utopia/Messaging/Adapter/Push/OneSignal.php
Comment thread src/Utopia/Messaging/Adapter/Push/OneSignal.php Outdated
Comment on lines +84 to +86
if (!\is_null($message->getContentAvailable())) {
$payload['content_available'] = true;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 content_available condition inverts intent when explicitly set to false

The guard !\is_null($message->getContentAvailable()) is true for both true and false. If a caller explicitly constructs Push with contentAvailable: false, the adapter still writes 'content_available' => true into the payload, silently enabling silent/background push on OneSignal devices when the caller's intent was the opposite. The fix is to check the boolean value directly: if ($message->getContentAvailable()).

@deepshekhardas deepshekhardas force-pushed the feat/7726-onesignal-adapter branch from 6f22bda to ec8ec39 Compare July 10, 2026 05:01
@deepshekhardas

Copy link
Copy Markdown
Author

Following up - OneSignal push adapter. Ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant