8000 [Notifier] Extend FakeSms & FakeChat notifier · Issue #40625 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Notifier] Extend FakeSms & FakeChat notifier #40625

New issue

Have a question about this project? Sign up for a fre 8000 e 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
2 of 6 tasks
OskarStark opened this issue Mar 29, 2021 · 18 comments
Closed
2 of 6 tasks

[Notifier] Extend FakeSms & FakeChat notifier #40625

OskarStark opened this issue Mar 29, 2021 · 18 comments
Labels
Notifier

Comments

@OskarStark
Copy link
Contributor
OskarStark commented Mar 29, 2021 •
edited
Loading

Based on this comment

  • fakesms+email should receive a mailer service id ✅ see initial PR [Notifier] [FakeSms] Add the bridge #39949
  • fakesms+logger should receive a logger service id + an optional channel ?? 🤔
  • fakesms+database should receive an entity manager service id + options ?? 🤔

And we end up with one Factory (like https://github.com/symfony/symfony/blob/5.x/src/Symfony/Component/Mailer/Bridge/Amazon/Transport/SesTransportFactory.php) and 6 transports

As another follow up we could create a symfony/fake-chat-notifier:

@JamesHemery @noniagriconomie @ke20 anybody? 😄
@noniagriconomie
Copy link
Contributor

@OskarStark are/should those transport only for dev|test env?
related to https://github.com/symfony/symfony/pull/39949/files#r564443227
👍 to leverage more "storage" for those fake notifier :)

@JamesHemery
Copy link
Contributor

@OskarStark Instead of having two separate Notifiers, wouldn't it be more interesting to have a fake Notifier that supports chat and sms?

@OskarStark
Copy link
Contributor Author

We can, but what would be the benefit? We need 2 dedicated transport factories anyway which are registered in the framework bundle plus the package is already named symfony/fake-sms-notifier

@OskarStark
Copy link
Contributor Author

@OskarStark are/should those transport only for dev|test env?

Yes

@JamesHemery
Copy link
Contributor

We can, but what would be the benefit? We need 2 dedicated transport factories anyway which are registered in the framework bundle plus the package is already named symfony/fake-sms-notifier

Yes indeed, no significant advantage, let's keep it that way.

@ke20
Copy link
ke20 commented Mar 29, 2021

Sorry @OskarStark I didn't follow last subjects since last time.

You're looking for someone to get and handle one subject above ?

If so, I check if I can take the subject fakesms+database and I've back to you asap for the answer.

@OskarStark
Copy link
Contributor Author
OskarStark commented Mar 29, 2021 •
edited
Loading

@ke20 glad to hear that 🤞🏻👌🏻
Go ahead with the database 🔥

@JamesHemery
Copy link
Contributor

I'll try to submit a PR for the logger channel next week.

@OskarStark OskarStark changed the title [Notifier] Extend FakeSms notifier [Notifier] Extend FakeSms & FakeChat notifier Mar 31, 2021
@OskarStark
Copy link
Contributor Author

any progress here @JamesHemery @ke20 ? 😃

@ke20
Copy link
ke20 commented May 7, 2021 •
edited
Loading

any progress here @JamesHemery @ke20 ? smiley

Hi @OskarStark !

Not really again... I've start a first little approach but when I wanted to test that I had some errors, but I don't know why at the moment.

My error is :

In FakeSmsTransportFactory.php line 30:
                                                                                                                                                                                                                   
  Argument 2 passed to Symfony\Component\Notifier\Bridge\FakeSms\FakeSmsTransportFactory::__construct() must implement interface Doctrine\ORM\EntityManagerInterface, instance of Symfony\Component\HttpClient\Cu  
  rlHttpClient given, called in /home/kevin/Workspaces/web/perso/contributions/test_5x/var/cache/dev/ContainerMSXxNgh/getNotifier_TransportFactory_FakesmsService.php on line 34

I don't understand why EntityManagerInterface is not injected into the class FakeSmsTransportFactory automatically when the MailerInterface is...

Someone know if an explicit configuration is required ?

Edit: An extract of my changements :

final class FakeSmsTransportFactory extends AbstractTransportFactory
{
   protected $mailer;
   protected $entityManager;

   public function __construct(MailerInterface $mailer, EntityManagerInterface $entityManager)
   {
       parent::__construct();

       $this->mailer = $mailer;
       $this->entityManager = $entityManager;
   }

@OskarStark
Copy link
Contributor Author

Check FrameworkExtension.php

@ke20
Copy link
ke20 commented May 7, 2021

Ok thank you I'll look that !
< 8000 form class="js-comment-update" id="issuecomment-834220205-edit-form" data-turbo="false" action="/symfony/symfony/issue_comments/834220205" accept-charset="UTF-8" method="post">

@OskarStark
Copy link
Contributor Author

Any news?

@JamesHemery
Copy link
Contributor

@OskarStark No I can't right now, maybe later :/

@noniagriconomie
Copy link
Contributor
noniagriconomie commented Jul 15, 2021 •
edited
Loading

@OskarStark I'll be glad to work on the fakesms+logger + fakesms+database ones as I use an sms notifier

For the database, how can/should we configure the storage?

Will provide a PR soon

@noniagriconomie noniagriconomie mentioned this issue Jul 15, 2021
Merged
@OskarStark
Copy link
Contributor Author

For the database, how can/should we configure the storage?

To be honest I am not really sure about the way to go, proposals?

@noniagriconomie
Copy link
Contributor

To be honest I am not really sure about the way to go, proposals?

clone the messenger database doctrine logic? but it seems huge :s

@OskarStark
Copy link
Contributor Author

Too big 😄

I think we should not implement it for now logger and email is very helpful

@noniagriconomie noniagriconomie mentioned this issue Jul 19, 2021
Merged
OskarStark added a commit that referenced this issue Jul 31, 2021
@OskarStark
feature #42123 [Notifier] Add FakeSMS Logger transport (noniagriconomie)
350674e 
This PR was merged into the 5.4 branch.

Discussion
----------

[Notifier] Add FakeSMS Logger transport

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Sub part of #40625
| License       | MIT
| Doc PR        | WIP

Friendly ping `@OskarStark`

As commented [here](#40625 (comment)) I use mainly the sms transport, thus wanted to work on the fake sms. This PR adds the `logger`.

For the part `an optional channel`, how can we get to here? dymanically retreiving the proper logger based on the dsn config?

Commits
-------

2596a72 Add FakeSMS Logger transport
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this issue Jul 31, 2021
@OskarStark
feature #42123 [Notifier] Add FakeSMS Logger transport (noniagriconomie)
eac054a 
This PR was merged into the 5.4 branch.

Discussion
----------

[Notifier] Add FakeSMS Logger transport

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Sub part of #40625
| License       | MIT
| Doc PR        | WIP

Friendly ping `@OskarStark`

As commented [here](symfony/symfony#40625 (comment)) I use mainly the sms transport, thus wanted to work on the fake sms. This PR adds the `logger`.

For the part `an optional channel`, how can we get to here? dymanically retreiving the proper logger based on the dsn config?

Commits
-------

2596a724e4 Add FakeSMS Logger transport
OskarStark added a commit that referenced this issue Aug 4, 2021
@OskarStark
feature #42188 [Notifier] Add FakeChat Logger transport (noniagricono…
7c1820d 
…mie)

This PR was merged into the 5.4 branch.

Discussion
----------

[Notifier] Add FakeChat Logger transport

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Sub part of #40625
| License       | MIT
| Doc PR        | WIP

Refs #42123 for Notifier FakeChat

Commits
-------

2bfe06f Add FakeChat Logger transport
@94noni 94noni mentioned this issue Nov 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Notifier
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@OskarStark @ke20 @noniagriconomie @carsonbot @JamesHemery
0