Skip to content

[Notifier] add support for gatewayapi-notifier #38685

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
Closed
wants to merge 53 commits into from
Closed

[Notifier] add support for gatewayapi-notifier #38685

wants to merge 53 commits into from

Conversation

PGLongo
Copy link
Contributor

@PGLongo PGLongo commented Oct 22, 2020 •
edited
Loading

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
License MIT
Doc PR symfony-docs/pull/14463

I've added support to GatewayAPI with Notifier

Nyholm
Nyholm reviewed Oct 22, 2020
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Wow. Thank you. What a great first contribution.

Could I ask you to add a some tests to this feature too?

@PGLongo
Copy link
Contributor Author

PGLongo commented Oct 23, 2020

@Nyholm I will add some tests

@nicolas-grekas nicolas-grekas added this to the 5.2 milestone Oct 23, 2020
@PGLongo PGLongo mentioned this pull request Oct 23, 2020
Merged
@PGLongo PGLongo requested a review from Nyholm October 23, 2020 09:11
@nicolas-grekas nicolas-grekas modified the milestones: 5.2, 5.x Oct 23, 2020
@PGLongo
Copy link
Contributor Author

PGLongo commented Oct 24, 2020

@Nyholm, are the test ok?

@PGLongo
Copy link
Contributor Author

PGLongo commented Oct 27, 2020

I've forgotten to mention that I've added the recipe

Nyholm
Nyholm reviewed Oct 30, 2020
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Im happy with this after one small change.

Thank you

@PGLongo
Copy link
Contributor Author

PGLongo commented Oct 30, 2020

Thank you very much! I'm so happy to contribute to Symfony after years of usage.

Nyholm
Nyholm approved these changes Oct 31, 2020
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you.

Could you rebase this so it shows that the tests are green?

PGLongo and others added 9 commits December 15, 2020 12:05
@PGLongo @OskarStark
Update src/Symfony/Component/Notifier/Bridge/GatewayApi/composer.json
f9bbdb7 
Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
@PGLongo @OskarStark
Update src/Symfony/Component/Notifier/Bridge/GatewayApi/composer.json
9506724 
Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
@PGLongo @OskarStark
Update src/Symfony/Component/Notifier/Bridge/GatewayApi/Tests/Gateway…
8ae74ff 
…ApiTransportFactoryTest.php

Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
@PGLongo @OskarStark
Update src/Symfony/Component/Notifier/Bridge/GatewayApi/Tests/Gateway…
0e069b2 
…ApiTransportFactoryTest.php

Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
@PGLongo @OskarStark
Update src/Symfony/Component/Notifier/Bridge/GatewayApi/Tests/Gateway…
23fc6ef 
…ApiTransportTest.php

Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
@PGLongo @OskarStark
Update src/Symfony/Component/Notifier/Bridge/GatewayApi/Tests/Gateway…
d41d30e 
…ApiTransportFactoryTest.php

Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
@PGLongo @OskarStark
Update src/Symfony/Component/Notifier/Bridge/GatewayApi/Tests/Gateway…
09613da 
…ApiTransportFactoryTest.php

Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
@PGLongo
Merge remote-tracking branch 'upstream/5.x' into 5.x
b8412ee
@PGLongo
Use UnsupportedMessageTypeException instead of LogicException in Gate…
fe9473d 
…wayApiTransport
OskarStark
OskarStark approved these changes Dec 15, 2020
View reviewed changes
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.

Looks good to me @fabpot 👍

@PGLongo @OskarStark
Update src/Symfony/Component/Notifier/Bridge/GatewayApi/Tests/Gateway…
24fac37 
…ApiTransportFactoryTest.php

Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
@fabpot
Copy link
Member

fabpot commented Dec 17, 2020

@PGLongo Can you rebase to get rid of the merge commit? Thank you.

@PGLongo
Copy link
Contributor Author

PGLongo commented Dec 17, 2020

@fabpot I will try this weekend. I have a lot of conflicts and I don't have the time to fix them right now.

git rebase upstream/5.x 

Auto-merging src/Symfony/Component/Notifier/Transport.php
CONFLICT (content): Merge conflict in src/Symfony/Component/Notifier/Transport.php
Auto-merging src/Symfony/Bundle/FrameworkBundle/Resources/config/notifier_transports.php
CONFLICT (content): Merge conflict in src/Symfony/Bundle/FrameworkBundle/Resources/config/notifier_transports.php
Auto-merging src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
CONFLICT (content): Merge conflict in src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
error: could not apply 2575ca2b12... [Notifier] add support for gatewayapi-notifier
Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 2575ca2b12... [Notifier] add support for gatewayapi-notifier```

@PGLongo
Copy link
Contributor Author

PGLongo commented Dec 18, 2020

Please can someone help me with the rebase to get rid of the merge commit as asked by @fabpot? @Nyholm , @nicolas-grekas, or @OskarStark is it correct to have a lot of merge conflicts doing:

git rebase upstream/5.x

Have I to do something else?

@OskarStark
Copy link
Contributor

Sure, can you please invite me to your repo?

@OskarStark OskarStark mentioned this pull request Dec 18, 2020
Merged
@PGLongo PGLongo closed this Dec 20, 2020
OskarStark added a commit that referenced this pull request Jan 14, 2021
@OskarStark
feature #39568 [Notifier] Add GatewayApi bridge (Piergiuseppe Longo)
e55762d 
This PR was merged into the 5.3-dev branch.

Discussion
----------

[Notifier] Add GatewayApi bridge

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Replaces #38685
| License       | MIT
| Doc PR        | symfony/symfony-docs#14463
| Recipe PR        | symfony/recipes#864

Initial PR by @PGLongo

Commits
-------

6b9f721 [Notifier] Add GatewayApi bridge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@stof stof stof left review comments

@jderusse jderusse jderusse left review comments

@fabpot fabpot fabpot approved these changes

@OskarStark OskarStark OskarStark approved these changes

@Nyholm Nyholm Nyholm approved these changes

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

Successfully merging this pull request may close these issues.
8 participants
@PGLongo @Nyholm @fabpot @OskarStark @nicolas-grekas @stof @jderusse @carsonbot