feat: Add type property to Media Class#224
Conversation
Yi-pixel
force-pushed
the
add-media-type
branch
from
28e798e to
69e8b98
Compare
When parsing messages, other types of messages have separate classes, but for media messages, because there is too much overlap, including: sticker, image, document, audio, video, voice, and multiple types, they all use Media Class. However, there is no original type attribute in Media to confirm the type of the message.
There was a problem hiding this comment.
Pull request overview
Adds an explicit “media message type” attribute to webhook Notification\Media instances so consumers can distinguish between overlapping media notifications (image/document/sticker/audio/video/voice) without relying solely on the shared class.
Changes:
- Introduce
MediaTypeenum to represent supported media message types. - Extend
Notification\Mediato store and expose atype()property. - Populate and assert the media type when building media notifications from webhook payloads.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/Unit/WebHook/NotificationFactoryTest.php | Adds assertions that built media notifications expose the expected MediaType. |
| src/WebHook/Notification/MessageNotificationFactory.php | Passes the webhook message type into Notification\Media as a MediaType. |
| src/WebHook/Notification/Media.php | Adds MediaType $type storage and accessor on the media notification class. |
| src/Message/Media/MediaType.php | New enum defining supported media types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -22,7 +26,8 @@ public function __construct( | |||
| string $sha256, | |||
| string $filename, | |||
| string $caption, | |||
| string $received_at_timestamp | |||
| string $received_at_timestamp, | |||
| MediaType $type | |||
| ) { | |||
There was a problem hiding this comment.
Changing Media's constructor signature by adding a required $type parameter is a backward-incompatible change for any consumers instantiating Notification\Media directly. Consider making the new argument optional (e.g., nullable with a sensible fallback like an UNKNOWN enum value), or accept the raw type string and construct MediaType internally so existing call sites can remain valid.
| return new Media( | ||
| $message['id'], | ||
| new Support\Business($metadata['phone_number_id'], $metadata['display_phone_number']), | ||
| $message[$message['type']]['id'], | ||
| $message[$message['type']]['mime_type'], | ||
| $message[$message['type']]['sha256'], | ||
| $message[$message['type']]['filename'] ?? '', | ||
| $message[$message['type']]['caption'] ?? '', | ||
| $message['timestamp'] | ||
| $message['timestamp'], | ||
| new MediaType($message['type']) | ||
| ); |
There was a problem hiding this comment.
MessageNotificationFactory now needs to know about MediaType and constructs it itself. To reduce coupling (and keep enum parsing/validation in one place), consider passing the media type string into Notification\Media and letting Media create the MediaType internally (similar to how Support\Conversation wraps its string type into ConversationType).
| public function test_build_from_payload_can_build_an_image_notification() | ||
| { | ||
| $payload = json_decode('{ | ||
| "object": "whatsapp_business_account", | ||
| "entry": [{ | ||
| "id": "WHATSAPP_BUSINESS_ACCOUNT_ID", | ||
| "changes": [{ | ||
| "value": { | ||
| "messaging_product": "whatsapp", | ||
| "metadata": { | ||
| "display_phone_number": "PHONE_NUMBER", | ||
| "phone_number_id": "PHONE_NUMBER_ID" | ||
| }, | ||
| "contacts": [{ | ||
| "profile": { | ||
| "name": "NAME" | ||
| }, | ||
| "wa_id": "WHATSAPP_ID" | ||
| }], | ||
| "messages": [{ | ||
| "from": "PHONE_NUMBER", | ||
| "id": "wamid.ID", | ||
| "timestamp": "1669233778", | ||
| "type": "image", | ||
| "image": { | ||
| "caption": "CAPTION_TEXT", | ||
| "mime_type": "image/jpeg", | ||
| "sha256": "IMAGE_HASH", | ||
| "id": "IMAGE_ID" | ||
| } | ||
| }] | ||
| }, | ||
| "field": "messages" | ||
| }] | ||
| }] | ||
| }', true); | ||
|
|
||
| $notification = $this->notification_factory->buildFromPayload($payload); | ||
|
|
||
| $this->assertInstanceOf(Notification\Media::class, $notification); | ||
| $this->assertEquals('IMAGE_ID', $notification->imageId()); | ||
| $this->assertEquals('IMAGE_HASH', $notification->sha256()); | ||
| $this->assertEquals('image/jpeg', $notification->mimeType()); | ||
| $this->assertEquals('CAPTION_TEXT', $notification->caption()); | ||
| $this->assertEquals(MediaType::IMAGE(), $notification->type()); | ||
| } | ||
|
|
There was a problem hiding this comment.
MediaType now includes AUDIO/VIDEO/VOICE, but NotificationFactoryTest only asserts the new Media::type() behavior for image/document/sticker. Adding at least one webhook payload test for audio/video/voice would help ensure the type mapping is correct across all supported media types and prevent regressions in the shared Media path.
When parsing messages, other types of messages have separate classes, but for media messages, because there is too much overlap, including: sticker, image, document, audio, video, voice, and multiple types, they all use Media Class.
However, there is no original type attribute in Media to confirm the type of the message.