Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (see https://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
• Features and deprecations must be submitted against the 5.4 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@OskarStark I updated all your suggestions. Could you take a look again?

By the way, Travis CI build is always failed in the FrameworkBundle tests because the package hasn't loaded yet. Could you advise what should I do for this case?

Thanks for your quick review!

Hi @vuphuong87

Thanks for providing this new notifier bridge. Code looks perfect already.

By the way, Travis CI build is always failed in the FrameworkBundle tests because the package hasn't loaded yet. Could you advise what should I do for this case?

To fix this issue, you need to add the new bridge as a composer dev requirement in the FrameworkBundle: https://github.com/symfony/symfony/blob/5.4/src/Symfony/Bundle/FrameworkBundle/composer.json#L70
Otherwise the factory service for this bridge will be removed: https://github.com/symfony/symfony/blob/5.4/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L2468

@OskarStark Not sure how it works, but creating the repository for the MessageMedia bridge (symfony/message-media-notifier) before we merge this PR is not possible, is it? Alternatively I could change this test:
https://github.com/symfony/symfony/blob/5.4/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php#L1883
and trigger a warning instead of letting the test fail in case an expected factory service is not found. It could look like this: 468e44b

@jschaedl I think possible merge while we do not have 📦 like additional repository.

@OskarStark know much more than me :)

@jschaedl My thought is the tests should be there to make sure the integration is correct or reviewers will always need to remind new contributors like me 😛 Do you know how did it work with the previous bridges?

I think we can ignore this error for new bridges, it will be green after merging.

WDYT @chalasr ?

EDIT:
Oh no, it will be green after merge and the release of the package. So I think we should go this way @jschaedl proposed

@vuphuong87 Can you check why the tests fail?

PHPUnit 9.5.6 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Testing /var/www/symfony/symfony/src/Symfony/Component/Notifier/Bridge/MessageMedia
.....SS....R..........                                            22 / 22 (100%)

Time: 00:00.033, Memory: 10.00 MB

There was 1 risky test:

1) Symfony\Component\Notifier\Bridge\MessageMedia\Tests\MessageMediaTransportTest::testExceptionIsThrownWhenHttpSendFailedProvider
This test did not perform any assertions

/var/www/symfony/symfony/src/Symfony/Component/Notifier/Bridge/MessageMedia/Tests/MessageMediaTransportTest.php:75

Hey, are you ready to finish your PR @vuphuong87 ? 👍

@fabpot @StaffNowa My bad. I put a bad naming for the data provider which phpunit think it was a test. I fixed it now.

Looks like there are flaky tests for 7.2 & 8.0.
For 8.0 (low-deps) I guess it will be good when I rebase with 5.4 branch given #41868 were merged.

@OskarStark still get the same errors about Telnyx in the latest build. Look like it is because of the subsplit not being configured yet (as @wouterj commented in the PR).

@vuphuong87 telnyx not yet pushed to packagist. Which error did you get?

symfony/recipes#951

Can you please rebase, afterwards I will merge this PR, thanks

@StaffNowa Yep, I think that is the case. The error is by the way:
Did you forget to add the TransportFactory: "Telnyx" to the $classToServices array in the FrameworkBundleExtension?

@OskarStark I rebased it. Thank you very much!!

can you please fix the test?

@OskarStark I pushed a new fix for it. Turn out it throws another exception for low-deps so I added another catch: cb524ab

Could you review again. Really it works well for now.

Yes lets see, what the tests say.

btw. can you please check, why your commuter e-mails not associated with your Github account:

Thanks

@OskarStark I messed up with my work email. Updated the PR again, my face is now shown up :p Thanks.

Thanks for your work on this new feature!

Thanks a lot for you feedback and handling this PR @OskarStark

Very welcome, thanks for your contribution!