Skip to content

Navigation Menu

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

Appearance settings

[Notifier] Add SentMessage additional info #59481

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

Merged
merged 1 commit into from
Jan 25, 2025

Conversation

mRoca
Copy link
Contributor

@mRoca mRoca commented Jan 11, 2025

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #49793
License MIT

This PR allows adding additional data into Notifier/Message/SentMessage.php, based on (and using) HttpClient Response's info. The initial need behind this feature is to be able to access to some data of the Transport's response (e.g.: nbSms and cost for allmysms, or totalCreditsRemoved for OVH Cloud).

Original (closed) PR: #51746

Some points to discuss:

@mRoca mRoca requested a review from OskarStark as a code owner January 11, 2025 18:31
@carsonbot carsonbot added this to the 7.3 milestone Jan 11, 2025
@mRoca mRoca force-pushed the feat/notifier/sent-message-info branch 3 times, most recently from 0dff16e to ffdf909 Compare January 11, 2025 18:53
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Putting the full info array is a bad idea to me. it contains unwanted things like the "pause" closure for example.

I think it's a good idea to add extra info, but I'd expect the transport to know which items are useful, and where they come from. No generic solution but adhoc ones: one for ovh, another for allmysms, etc.

@mRoca
Copy link
Contributor Author

mRoca commented Jan 13, 2025

@nicolas-grekas indeed, I only saw scalar data in ResponseInterface's getInfo result. If a callable can be returned too, that can lead to unwanted behavior.

WDYT would be the best ? Totally removing the setHttpResponseInfo shortcut, or keeping it and only extract raw scalar data (status code / headers / body as array) ?

@nicolas-grekas
Copy link
Member

I'd expect the transport to know which items are useful, and where they come from. No generic solution but adhoc ones: one for ovh, another for allmysms, etc.

@mRoca mRoca force-pushed the feat/notifier/sent-message-info branch from ffdf909 to 74aa75a Compare January 13, 2025 10:01
@mRoca
Copy link
Contributor Author

mRoca commented Jan 13, 2025

@nicolas-grekas method setHttpResponseInfo removed 👍🏻

for easier PR history, the method was:

    /**
     * As many of the Transports are using HTTP requests,
     * this shortcut attaches an HTTP response to the message in a standardised way.
     */
    public function setHttpResponseInfo(ResponseInterface $response): void
    {
        $this->setInfo('http_response', $response->getInfo() + ['body' => $response->toArray(false)]);
    }

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

would you be able to populate that info for a few bridges you know about? it'd be nice to showcase the benefit of this!

@mRoca mRoca force-pushed the feat/notifier/sent-message-info branch 2 times, most recently from 19b6b8a to ef126c3 Compare January 13, 2025 11:38
@mRoca
Copy link
Contributor Author

mRoca commented Jan 13, 2025

I've added implementation for both OvhCloud & allmysms (@Vincentig's issue) transports

@mRoca mRoca force-pushed the feat/notifier/sent-message-info branch 3 times, most recently from 909b832 to 70b1c96 Compare January 13, 2025 12:32
Copy link
Member

@nicolas-grekas nicolas-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 with minor changes

@mRoca mRoca force-pushed the feat/notifier/sent-message-info branch 2 times, most recently from 58c0e85 to 068cccf Compare January 24, 2025 09:31
@mRoca mRoca force-pushed the feat/notifier/sent-message-info branch from 068cccf to a50f38c Compare January 24, 2025 10:08
@@ -48,6 +48,9 @@ public function supports(MessageInterface $message): bool
return $message instanceof SmsMessage && (null === $message->getOptions() || $message->getOptions() instanceof AllMySmsOptions);
}

/**
* @See https://doc.allmysms.com/api/en/#api-SMS-sendsimple
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @See https://doc.allmysms.com/api/en/#api-SMS-sendsimple
* @see https://doc.allmysms.com/api/en/#api-SMS-sendsimple

@fabpot
Copy link
Member

fabpot commented Jan 25, 2025

Thank you @mRoca.

@fabpot fabpot merged commit e787b6f into symfony:7.3 Jan 25, 2025
5 of 11 checks passed
@fabpot fabpot mentioned this pull request May 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Notifier] SentMessage add debug property
4 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.