Skip to content

Update multiple services example #13265

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Mar 12, 2020
Merged

Update multiple services example #13265

merged 6 commits into from
Mar 12, 2020

Conversation

ker0x
Copy link
Contributor

@ker0x ker0x commented Feb 27, 2020

This PR propose to replace SwiftMailer with the Mailer component and to add the Logger service for a better understanding how to inject multiple services

@OskarStark
Copy link
Contributor

I like the idea but would not add the Logger in this example, yes it makes sense but complicated the example itself

@xabbuh xabbuh added this to the 4.4 milestone Mar 2, 2020
@ker0x
Copy link
Contributor Author

ker0x commented Mar 3, 2020

Hello @OskarStark , thank you for your feedback!

I have 3 propositions:

  1. Removing the Logger from the example. This leave us with a try/catch that only return a boolean, it's a bit strange IMHO
  2. Keeping the Logger and update the intro message of the Handling Multiple Services to specify that we want to log any exception that can be triggered when sending the email.
  3. Replacing the Logger with an other component, but which one ?

@OskarStark
Copy link
Contributor

OskarStark commented Mar 3, 2020

Please use the same example as before, no need for a try catch here, it’s just an example and should not distract by things which are not needed.

Using

// ...

Is fine too, meaning there could be some other code parts around. Thanks

Edit:
You can remove the return type too please

If you have further questions or sth is unclear please feel free to ping me again and I can provide the full example. I am currently on a phone 📲

Copy link
Contributor

@HeahDude HeahDude 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, thanks

}
}

This needs the ``MessageGenerator`` *and* the ``Swift_Mailer`` service. That's no
This needs the ``MessageGenerator`` *and* the ``Mailer`` service. That's no
Copy link
Contributor

Choose a reason for hiding this comment

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

MailerInterface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idk, I'm a little sceptical about this change. We call the Mailer service through the MailerInterface no?

Would't this sound better ?

This needs the MessageGenerator and the Mailer service. That's no
problem, we ask them by type-hinting their class and interface names!

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds better indeed

javiereguiluz and others added 5 commits March 12, 2020 14:58
…er (lyrixx)

This PR was merged into the 3.4 branch.

Discussion
----------

Fixed CoverageListener usage with custom SUT solver

Commits
-------

aeb4053 Fixed CoverageListener usage with custom SUT solver
* 3.4:
  Fixed CoverageListener usage with custom SUT solver
formating improve
This PR was submitted for the 5.0 branch but it was merged into the 4.4 branch instead (closes symfony#13237).

Discussion
----------

Update messenger.rst

formating improve

<!--

If your pull request fixes a BUG, use the oldest maintained branch that contains
the bug (see https://symfony.com/roadmap for the list of maintained branches).

If your pull request documents a NEW FEATURE, use the same Symfony branch where
the feature was introduced (and `master` for features of unreleased versions).

-->

Commits
-------

73e088c Update messenger.rst
This PR propose to replace SwiftMailer with the Mailer component and to add the Logger service for a better understanding how to inject multiple services
@javiereguiluz javiereguiluz changed the base branch from 5.0 to 4.4 March 12, 2020 14:03
@javiereguiluz
Copy link
Member

Thanks Romain.

@javiereguiluz javiereguiluz merged commit 5c48d08 into symfony:4.4 Mar 12, 2020
@ker0x ker0x deleted the patch-1 branch March 12, 2020 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants