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] Rework/streamline bridges (5.2)#39428

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
fabpot merged 1 commit intosymfony:5.2fromOskarStark:notifier-5.2
Dec 18, 2020

Conversation

@OskarStark
Copy link
Contributor

@OskarStarkOskarStark commentedDec 10, 2020•
edited
Loading

QA
Branch?5.2
Bug fix?no
New feature?no
Deprecations?no
Tickets---
LicenseMIT
Doc PR---

This PR

  • add missing tests
  • pull up scheme check (check scheme first and then for required options)
  • streamlines README.md files

While working on adding tests forsymfony/esendex-notifier I noticed that theEsendexTransport has the following signature:

publicfunction__construct(string$token,string$accountReference,string$from,HttpClientInterface$client =null,EventDispatcherInterface$dispatcher =null)

and is resolved by theEsendexTransportFactory like:

$token =$this->getUser($dsn).':'.$this->getPassword($dsn);

but theREADME exposes the DSN like:

esendex://EMAIL:PASSWORD@default?accountreference=ACCOUNT_REFERENCE&from=FROM

as this Bridge is experimental in5.2I propose to change the transport signature like, because to me it is more email/password like described in the readme than a "token":

- public function __construct(string $token, string $accountReference, string $from, HttpClientInterface $client = null,EventDispatcherInterface $dispatcher = null)+ public function __construct(string $email, string $password, string $accountReference, string $from, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null)

What do you think?

cc@odolbeau as you provided the Esendex bridge.

Copy link
Member

@derrabusderrabus left a comment

Choose a reason for hiding this comment

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

Taking back my approval because Oskar says the PR is not ready. 🙃

OskarStark reacted with thumbs up emojiOskarStark reacted with rocket emoji
@odolbeau
Copy link
Contributor

@OskarStark Regarding the proposed change for theEsendexTransport, it totally makes sense! 👌

Thanks for improving those components. :)

@OskarStark
Copy link
ContributorAuthor

@OskarStark Regarding the proposed change for the EsendexTransport, it totally makes sense! 👌

Thanks for improving those components. :)

Ok, thank you for your feedback, I will do this in a separate PR after this one is merged 👍

@OskarStarkOskarStarkforce-pushed thenotifier-5.2 branch 3 times, most recently from17e3a56 to5914bdeCompareDecember 14, 2020 07:19
@OskarStark
Copy link
ContributorAuthor

@fabpot I would love to get a review/merge here, so I can proceed, thanks 👍

@fabpot
Copy link
Member

Thank you@OskarStark.

OskarStark reacted with thumbs up emoji

@fabpotfabpot merged commita566eee intosymfony:5.2Dec 18, 2020
@OskarStarkOskarStark deleted the notifier-5.2 branchDecember 18, 2020 08:01
OskarStark added a commit to OskarStark/symfony that referenced this pull requestDec 21, 2020
| Q             | A| ------------- | ---| Branch?       | 5.x, but BC BREAK for experimental bridge| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | ---| License       | MIT| Doc PR        | ---Based onsymfony#39428 (comment)* [ ] Update recipe* [ ] Update documentationcc@odolbeau as you provided the bridge
OskarStark added a commit to OskarStark/symfony that referenced this pull requestDec 21, 2020
| Q             | A| ------------- | ---| Branch?       | 5.x, but BC BREAK for experimental bridge| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | ---| License       | MIT| Doc PR        | ---Based onsymfony#39428 (comment)cc@odolbeau as you provided the bridge
OskarStark added a commit to OskarStark/symfony that referenced this pull requestDec 22, 2020
| Q             | A| ------------- | ---| Branch?       | 5.x, but BC BREAK for experimental bridge| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | ---| License       | MIT| Doc PR        | ---Based onsymfony#39428 (comment)cc@odolbeau as you provided the bridge
OskarStark added a commit to OskarStark/symfony that referenced this pull requestDec 22, 2020
| Q             | A| ------------- | ---| Branch?       | 5.x, but BC BREAK for experimental bridge| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | ---| License       | MIT| Doc PR        | ---Based onsymfony#39428 (comment)cc@odolbeau as you provided the bridge
OskarStark added a commit to OskarStark/symfony that referenced this pull requestDec 22, 2020
| Q             | A| ------------- | ---| Branch?       | 5.x, but BC BREAK for experimental bridge| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | ---| License       | MIT| Doc PR        | ---Based onsymfony#39428 (comment)cc@odolbeau as you provided the bridge
OskarStark added a commit to OskarStark/symfony that referenced this pull requestDec 25, 2020
| Q             | A| ------------- | ---| Branch?       | 5.x, but BC BREAK for experimental bridge| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | ---| License       | MIT| Doc PR        | ---Based onsymfony#39428 (comment)cc@odolbeau as you provided the bridge
OskarStark added a commit to OskarStark/symfony that referenced this pull requestDec 28, 2020
| Q             | A| ------------- | ---| Branch?       | 5.x, but BC BREAK for experimental bridge| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | ---| License       | MIT| Doc PR        | ---Based onsymfony#39428 (comment)cc@odolbeau as you provided the bridge
fabpot added a commit that referenced this pull requestDec 29, 2020
… Mattermost and Esendex transport (OskarStark)This PR was squashed before being merged into the 5.3-dev branch.Discussion----------[Notifier] [BC BREAK] Change constructor signature for Mattermost and Esendex transport| Q             | A| ------------- | ---| Branch?       | 5.x, but BC BREAK for experimental bridge| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | ---| License       | MIT| Doc PR        | ---Based on#39428 (comment)cc@odolbeau as you provided the bridgeCommits-------c5b9acf [Notifier] [BC BREAK] Change constructor signature for Mattermost and Esendex transport
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@derrabusderrabusAwaiting requested review from derrabus

@jderussejderusseAwaiting requested review from jderusse

Assignees

No one assigned

Projects

None yet

Milestone

5.2

Development

Successfully merging this pull request may close these issues.

6 participants

@OskarStark@odolbeau@fabpot@jderusse@derrabus@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp