Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Notifier] Add support for building SmsEvent by dlr_code and RemoteEvent for other LOX24 webhook event types#61778

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:7.4fromalebedev80:7.4
Sep 24, 2025

Conversation

@alebedev80
Copy link
Contributor

@alebedev80alebedev80 commentedSep 17, 2025
edited by nicolas-grekas
Loading

QA
Branch?7.4
Bug fix?yes
New feature?yes
Deprecations?no
IssuesFix #... (if applicable)
LicenseMIT

Summary

This PR enhances the LOX24RequestParser to provide robust SMS delivery webhook parsing with improved validation, dual status code support (DLR codes and status codes), and better error handling for SMS delivery events.

What it does

Before: The parser had basic SMS delivery webhook handling with limited validation and error handling.

After: The parser provides enhanced SMS delivery webhook processing:

  • sms.delivery &sms.delivery.dryrun - SMS delivery receipts with improved validation
  • Dual status code support: DLR codes (0-16) and transmission status codes (0, 100, etc.)
  • Better payload validation with detailed error messages
  • Non-delivery events are handled as generic RemoteEvent objects

Example Usage

Enhanced SMS Delivery Processing:

// ✅ SMS delivery with status code$deliveryPayload = ['id' =>'webhook-123','name' =>'sms.delivery','data' => ['id' =>'123','status_code' =>100]];$deliveryEvent =$parser->parse($deliveryRequest,$secret);// Returns: SmsEvent with DELIVERED status// ✅ SMS delivery with DLR code (takes priority over status_code)$dlrPayload = ['id' =>'webhook-456','name' =>'sms.delivery','data' => ['id' =>'456','dlr_code' =>1,'status_code' =>500]];$dlrEvent =$parser->parse($dlrRequest,$secret);// Returns: SmsEvent with DELIVERED status (DLR code 1 overrides status 500)// ✅ Pending delivery (returns null)$pendingPayload = ['id' =>'webhook-789','name' =>'sms.delivery','data' => ['id' =>'789','dlr_code' =>0]];$pendingEvent =$parser->parse($pendingRequest,$secret);// Returns: null (pending delivery)// ✅ Non-delivery events handled as RemoteEvent$otherPayload = ['id' =>'webhook-000','name' =>'custom.event','data' => ['some' =>'data']];$otherEvent =$parser->parse($otherRequest,$secret);// Returns: RemoteEvent with 'custom.event' name

Key Changes

1.Improved Payload Validation

  • Enhanced validation for required webhook fields:id,name,data
  • Better error messages with specific missing field details
  • Proper array validation for data payload

2.Smart Return Types

  • SMS delivery events (sms.delivery,sms.delivery.dryrun) returnSmsEvent objects with DELIVERED/FAILED status
  • Non-delivery events returnRemoteEvent objects for generic webhook handling
  • Maintainsnull return for pending deliveries (status_code=0 or dlr_code=0/2/4)

3.Enhanced SMS Delivery Validation

// SMS delivery events validate required fieldscase'sms.delivery':// Requires: payload.id, data.id, and either data.status_code OR data.dlr_code// Supports both LOX24 status codes and DLR codes// DLR codes take priority when both are present

4.Dual Status Code Support

  • DLR Codes: Priority support for LOX24's Delivery Report codes (0-16) with proper pending state handling
  • Status Codes: Fallback to transmission status codes (0, 100, 208, 400, etc.) when DLR unavailable
  • Smart Mapping: DLR codes 1→DELIVERED, 8/16→FAILED, 0/2/4→null (pending)

4.Comprehensive Testing

  • Tests for SMS delivery events with success/failure scenarios
  • DLR code and status code validation scenarios
  • Payload validation tests for required fields
  • Backward compatibility verification for existing delivery events
  • Type safety assertions (SmsEvent vsRemoteEvent)

Backward Compatibility

Fully backward compatible - existing consumers continue working without changes:

  • Samesms.delivery event handling behavior
  • Same status code and DLR code mapping logic
  • Same null return for pending messages (status_code=0, dlr_code=0/2/4)
  • Same SmsEvent object structure and methods
  • Enhanced support for both LOX24 status codes and DLR codes

Consumer Integration Example

#[AsRemoteEventConsumer('lox24_notifier')]class LOX24WebhookConsumerimplements ConsumerInterface{publicfunctionconsume(RemoteEvent$event):void    {if ($eventinstanceof SmsEvent) {match($event->getName()) {                SmsEvent::DELIVERED =>$this->handleDelivered($event),                SmsEvent::FAILED =>$this->handleFailed($event),            };        }else {// Handle other webhook events as generic RemoteEvent$this->handleGenericWebhook($event);        }    }}

Breaking Changes

⚠️Minor breaking changes (unlikely to affect most users):

  • Parser return type signature changed from?SmsEvent toSmsEvent|RemoteEvent|null
  • Exception messages updated with more detailed validation errors
  • Requires webhook payload to includeid field (LOX24 standard)
  • Non-delivery events now returnRemoteEvent instead of being rejected

Technical Implementation

  • Improved Validation: Enhanced payload validation with detailed error messages for missing fields
  • Factory Methods: Implemented specialized methods (createDeliveryEvent,createRemoteEvent, etc.)
  • Dual Code Support: Enhanced delivery event parsing to handle both LOX24 status codes and DLR codes
  • Smart Precedence: DLR codes take priority over status codes when both are present
  • Error Handling: Enhanced error messages with specific validation details
  • Null Handling: Proper handling of pending states (DLR codes 0/2/4, status code 0)

Testing

  • ✅ All existing tests pass (backward compatibility)
  • ✅ Added comprehensive test coverage for SMS delivery scenarios
  • ✅ Payload validation and error handling tests
  • ✅ DLR code and status code priority testing

This enhancement improves LOX24 SMS delivery webhook processing with better validation, dual status code support, and robust error handling while maintaining complete backward compatibility.

privatefunctioncreateIncomingSmsEvent(array$data,array$payload):RemoteEvent
{
if (!isset($data['from'],$data['to'],$data['text'])) {
thrownewRejectWebhookException(406,'Incoming SMS payload is malformed - missing required fields.');
Copy link
Member

Choose a reason for hiding this comment

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

Let's mention the missing field.

Copy link
Member

Choose a reason for hiding this comment

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

Not resolved

Copy link
Member

@fabpotfabpot left a comment

Choose a reason for hiding this comment

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

A lot of my suggestions were marked as resolved but they were not. I'm fine but can you add a small comment about why you don't think my comment was relevant/useful?

privatefunctioncreateIncomingSmsEvent(array$data,array$payload):RemoteEvent
{
if (!isset($data['from'],$data['to'],$data['text'])) {
thrownewRejectWebhookException(406,'Incoming SMS payload is malformed - missing required fields.');
Copy link
Member

Choose a reason for hiding this comment

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

Not resolved

'phone_number' =>'+1234567890',
'fraud_score' =>0.5,
],
default => [],// For email parsing events, minimal data is acceptable
Copy link
Member

Choose a reason for hiding this comment

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

Not resolved

@alebedev80
Copy link
ContributorAuthor

@fabpot please review

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

LGTM after some CS changes

@alebedev80
Copy link
ContributorAuthor

@nicolas-grekas thank you! i've committed yours changes. Please check it

Copy link
Member

@fabpotfabpot left a comment

Choose a reason for hiding this comment

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

Can you please unify all exception messages? One sentence, no dash, mentioning that the fields are required, ...

@stof
Copy link
Member

Should we really add new events that are specific to the LOX24 bridge instead of having a proper abstraction in the component that can implemented by the different bridges ?
Notifier bridges are not meant to be full SDKs for a given provider. They are meant to implement the shared abstraction of the component on top of a given provider.

You could totally implement a remote event parser as part of a LOX24 SDK supporting all their features (outside Symfony core), but this does not belong to the notifier bridge IMO.

Copy link
Member

@fabpotfabpot left a comment

Choose a reason for hiding this comment

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

Please, unify the error messages.

@alebedev80
Copy link
ContributorAuthor

@stof@nicolas-grekas i've removed LOX24 specific webhooks parsing logic. please check

@alebedev80
Copy link
ContributorAuthor

which conflicts i need to resolve?

@fabpot
Copy link
Member

which conflicts i need to resolve?

Maybe because the class name has changed fromLOX24RequestParser toLox24RequestParser?

@nicolas-grekas
Copy link
Member

the LOX24RequestParser file/class has been renamed to Lox24RequestParser
you'd need to fetch branch 7.4 and rebase on top of it to get the change
can you do it?

@alebedev80
Copy link
ContributorAuthor

Please, unify the error messages.

@fabpot done. please check this

Copy link
Member

@stofstof left a comment

Choose a reason for hiding this comment

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

As response to my previous comment#61778 (comment), you said you reverted the logic for LOX24-specific webhooks. However, both the code and the changelog entry you added tell me that you are still adding support for the LOX24-specific webhooks that don't match the Symfony abstraction of the notifier webhooks.

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

I suppose the PR title+description should also be synced now that the scope is reduced. LGTM otherwise, after some minor stuff.


privatefunctioncreateDeliveryEvent(array$data,array$payload): ?SmsEvent
{
if (!isset($data['id'])) {

Choose a reason for hiding this comment

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

just for confirmation: we already checked that id is present in payload, and id now also has to be inside payload['data'], is that correct?

Choose a reason for hiding this comment

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

@nicolas-grekasnicolas-grekas changed the title[Notifier][LOX24] Add support for all webhook event types[Notifier][LOX24] Enhance SMS delivery webhook parser with improved validationSep 24, 2025
@nicolas-grekasnicolas-grekas changed the title[Notifier][LOX24] Enhance SMS delivery webhook parser with improved validation[Notifier][LOX24] Add support for building SmsEvent by dlr_code and RemoteEvent for other LOX24 webhook event typesSep 24, 2025
@nicolas-grekasnicolas-grekas changed the title[Notifier][LOX24] Add support for building SmsEvent by dlr_code and RemoteEvent for other LOX24 webhook event types[Notifier] Add support for building SmsEvent by dlr_code and RemoteEvent for other LOX24 webhook event typesSep 24, 2025
@nicolas-grekas
Copy link
Member

Thank you@alebedev80.

alebedev80 reacted with heart emoji

@nicolas-grekasnicolas-grekas merged commitc6e3f9b intosymfony:7.4Sep 24, 2025
11 of 12 checks passed
returnnewSmsEvent($eventType,$data['id'],$payload);
}

privatefunctioncreateRemoteEvent(string$eventName,array$payload):RemoteEvent
Copy link
Member

Choose a reason for hiding this comment

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

Let's inline the code and remove this private method as it's only called once.

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

Reviewers

@fabpotfabpotfabpot requested changes

@stofstofstof left review comments

@OskarStarkOskarStarkOskarStark left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

7.4

Development

Successfully merging this pull request may close these issues.

6 participants

@alebedev80@stof@fabpot@nicolas-grekas@OskarStark@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp