8000 [Notifier] Document Notifier options in README files by alamirault · Pull Request #50349 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Notifier] Document Notifier options in README files #50349

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

alamirault
Copy link
Contributor
Q A
Branch? 6.3
Bug fix? no
New feature? no
Deprecations? no
Tickets Fix #...
License MIT
Doc PR symfony/symfony-docs#...

This PR add missing *Options documentations in notifier bridges readme

Documented bridges are:

Waiting your feedback 😄

I will create another PR for AmazonSnsOptions, BandwidthOptions, ClickSendOptions, MobytOptions, PlivoOptions, RingCentralOptions, SmsmodeOptions, TwilioOptions after this one

@alamirault alamirault requested a review from OskarStark as a code owner May 16, 2023 22:14
@carsonbot carsonbot added this to the 6.3 milestone May 16, 2023
@alamirault alamirault changed the title Feature/document notifier options readme [Notifier] Document notifier options readme May 16, 2023
Copy link
Contributor
@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

@nicolas-grekas do you agree with my comments?

10000
@OskarStark OskarStark changed the title [Notifier] Document notifier options readme [Notifier] Document Notifier options in README's May 17, 2023
@OskarStark OskarStark changed the title [Notifier] Document Notifier options in README's [Notifier] Document Notifier options in README files May 17, 2023
@alamirault alamirault requested a review from OskarStark May 18, 2023 16:46
nicolas-grekas
nicolas-grekas previously approved these changes May 18, 2023
$sms = new SmsMessage('+1411111111', 'My message');

$options = (new ContactEveryoneOptions())
->diffusionName('My label')
Copy link
Member

Choose a reason for hiding this comment

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

the method should be changed to accept a string also (would be consistent with the constructor of the transport)
@franckranaivo where can I find the doc that tells about this parameter?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, for the late answer. I picked these parameters (including diffusionName) from their manuals. But yes it should accept string. Here is the manual, idk if there is an english version.
https://ceo-be.multimediabs.com/attachments/hosted/lightApiManualsFR
They start to talk about these on page 9.

Apparently, it is useful for categorizing sms logs.

@nicolas-grekas nicolas-grekas force-pushed the feature/document-notifier-options-readme branch from 0e0dcbf to 6153f04 Compare May 20, 2023 16:32
@nicolas-grekas
Copy link
Member

Thank you @alamirault.

@nicolas-grekas nicolas-grekas merged commit 746c06b into symfony:6.3 May 20, 2023
@alamirault alamirault deleted the feature/document-notifier-options-readme branch May 20, 2023 21:39
nicolas-grekas added a commit that referenced this pull request May 22, 2023
…amirault)

This PR was squashed before being merged into the 6.3 branch.

Discussion
----------

[Notifier] Document Notifier options in README files

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Continue #50349 on 6.3 branch

Commits
-------

e770344 [Notifier] Document Notifier options in README files
nicolas-grekas added a commit that referenced this pull request May 22, 2023
… (alamirault)

This PR was squashed before being merged into the 5.4 branch.

Discussion
----------

[Notifier] Document Notifier options in README files 5.4

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Continue #50349 on 5.4 branch

Commits
-------

b670273 [Notifier] Document Notifier options in README files 5.4
@fabpot fabpot mentioned this pull request May 22, 2023
nicolas-grekas added a commit that referenced this pull request May 30, 2023
This PR was merged into the 6.3 branch.

Discussion
----------

[Notifier] Fix ContactEveryoneOptions

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Thanks to `@franckranaivo` in #50349 (comment)

Commits
-------

8dd9d93 [Notifier] Fix ContactEveryoneOptions
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.

5 participants
0