Skip to content

[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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 18, 2020
Merged

[Notifier] Rework/streamline bridges (5.2) #39428

merged 1 commit into from
Dec 18, 2020

Conversation

OskarStark
Copy link
Contributor

@OskarStark OskarStark commented Dec 10, 2020 •
edited
Loading

Q A
Branch? 5.2
Bug fix? no
New feature? no
Deprecations? no
Tickets ---
License MIT
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 for symfony/esendex-notifier I noticed that the EsendexTransport has the following signature:

symfony/src/Symfony/Component/Notifier/Bridge/Esendex/EsendexTransport.php

Line 36 in 613ac0c

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

and is resolved by the EsendexTransportFactory like:

symfony/src/Symfony/Component/Notifier/Bridge/Esendex/EsendexTransportFactory.php

Line 30 in 613ac0c

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

but the README exposes the DSN like:

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

as this Bridge is experimental in 5.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.

@carsonbot carsonbot added this to the 5.2 milestone Dec 10, 2020
@OskarStark OskarStark force-pushed the notifier-5.2 branch 3 times, most recently from 03d13d8 to fbd7182 Compare December 10, 2020 14:00
derrabus
derrabus requested changes Dec 10, 2020
View reviewed changes
Copy link
Member

@derrabus derrabus 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 OskarStark force-pushed the notifier-5.2 branch 2 times, most recently from ba65ec3 to a6a5ef6 Compare December 11, 2020 06:23
@odolbeau
Copy link
Contributor

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

Thanks for improving those components. :)

@OskarStark
Copy link
Contributor Author

@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 👍

@OskarStark OskarStark force-pushed the notifier-5.2 branch 3 times, most recently from 17e3a56 to 5914bde Compare December 14, 2020 07:19
@OskarStark OskarStark mentioned this pull request Dec 14, 2020
Merged
@OskarStark OskarStark requested a review from jderusse December 17, 2020 13:28
@OskarStark
Copy link
Contributor Author

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

@fabpot
Copy link
Member

fabpot commented Dec 18, 2020

Thank you @OskarStark.

@fabpot fabpot merged commit a566eee into symfony:5.2 Dec 18, 2020
@OskarStark OskarStark deleted the notifier-5.2 branch December 18, 2020 08:01
OskarStark added a commit to OskarStark/symfony that referenced this pull request Dec 21, 2020
@OskarStark
[Notifier] [Esendex] [BC BREAK] Change constructor signature
767e498 
| 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 symfony#39428 (comment)

* [ ] Update recipe
* [ ] Update documentation

cc @odolbeau as you provided the bridge
@OskarStark OskarStark mentioned this pull request Dec 21, 2020
Merged
OskarStark added a commit to OskarStark/symfony that referenced this pull request Dec 21, 2020
@OskarStark
[Notifier] [Esendex] [BC BREAK] Change constructor signature
cfb8a94 
| 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 symfony#39428 (comment)

cc @odolbeau as you provided the bridge
OskarStark added a commit to OskarStark/symfony that referenced this pull request Dec 22, 2020
@OskarStark
[Notifier] [Esendex] [BC BREAK] Change constructor signature
afff042 
| 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 symfony#39428 (comment)

cc @odolbeau as you provided the bridge
OskarStark added a commit to OskarStark/symfony that referenced this pull request Dec 22, 2020
@OskarStark
[Notifier] [Esendex] [BC BREAK] Change constructor signature
748db1c 
| 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 symfony#39428 (comment)

cc @odolbeau as you provided the bridge
@OskarStark OskarStark mentioned this pull request Dec 22, 2020
Merged
8 tasks
OskarStark added a commit to OskarStark/symfony that referenced this pull request Dec 22, 2020
@OskarStark
[Notifier] [Esendex] [BC BREAK] Change constructor signature
dabcf51 
| 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 symfony#39428 (comment)

cc @odolbeau as you provided the bridge
OskarStark added a commit to OskarStark/symfony that referenced this pull request Dec 25, 2020
@OskarStark
[Notifier] [Esendex] [BC BREAK] Change constructor signature
444120a 
| 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 symfony#39428 (comment)

cc @odolbeau as you provided the bridge
OskarStark added a commit to OskarStark/symfony that referenced this pull request Dec 28, 2020
@OskarStark
[Notifier] [Esendex] [BC BREAK] Change constructor signature
270e64d 
| 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 symfony#39428 (comment)

cc @odolbeau as you provided the bridge
fabpot added a commit that referenced this pull request Dec 29, 2020
@fabpot
feature #39592 [Notifier] [BC BREAK] Change constructor signature for…
ec9ed2f 
… 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 bridge

Commits
-------

c5b9acf [Notifier] [BC BREAK] Change constructor signature for Mattermost and Esendex transport
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@derrabus derrabus Awaiting requested review from derrabus

@jderusse jderusse Awaiting requested review from jderusse

Assignees
No one assigned
Labels
Notifier Status: Reviewed
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
6 participants
@OskarStark @odolbeau @fabpot @jderusse @derrabus @carsonbot