8000 [Mailer] bug - fix EsmtpTransport variable $code definition by kgnblg · Pull Request #51568 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Mailer] bug - fix EsmtpTransport variable $code definition #51568

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
Sep 6, 2023
Merged

[Mailer] bug - fix EsmtpTransport variable $code definition #51568

merged 1 commit into from
Sep 6, 2023

Conversation

kgnblg
Copy link
Contributor
@kgnblg kgnblg commented Sep 5, 2023
Q A
Branch? 6.3
Bug fix? yes
New feature? no
Deprecations? no
Tickets ---
License MIT
Doc PR ---

This PR contains a basic fix of a bug in SymfonyMailer component (Symfony\Component\Mailer\Transport\Smtp\EsmtpTransport class).

If we use EsmtpTransport like the given example, it needs to throw TransportException which has been defined in the handleAuth() of EsmtpTransport class. (Because we called setAuthenticators() with empty array and the function emptied the authenticators. We can imagine that we forget to set authenticators.)

$stream = new DummyStream();
$transport = new EsmtpTransport(stream: $stream);
$transport->setUsername('testuser');
$transport->setPassword('p4ssw0rd');
$transport->setAuthenticators([]);
..
..
$transport->send(...);

This code piece needs to throw following TransportException with code 504;

Failed to find an authenticator supported by the SMTP server, which currently supports: "plain", "login", "cram-md5", "xoauth2".

However it shows a PHP Error;

Undefined variable $code

I included a unit test function testConstructorWithEmptyAuthenticator() inside of the EsmtpTransportTest class with a fix of the problem.

@kgnblg
bug [mailer] fix EsmtpTransport variable $code definition
772050b 
the variable $code defined in the foreach loop, if there is no authenticators,
then the $code will not be defined and therefore the following TransportException
gives a PHP error.

the use-case defined as a unit test in EsmtpTransportTest class.
@carsonbot carsonbot added this to the 6.3 milestone Sep 5, 2023
@carsonbot
Copy link

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

fabpot
fabpot approved these changes Sep 5, 2023
View reviewed changes
Copy link
Member
@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

For 5.4?

@kgnblg
Copy link
Contributor Author
kgnblg commented Sep 5, 2023

I checked it for 5.4. This bug does not exist on 5.4.

@fabpot
Copy link
Member
fabpot commented Sep 6, 2023

Thank you @kgnblg.

@fabpot fabpot merged commit 29f427f into symfony:6.3 Sep 6, 2023
stof
stof reviewed Sep 6, 2023
View reviewed changes
src/Symfony/Component/Mailer/Transport/Smtp/EsmtpTransport.php
@@ -192,7 +193,6 @@ private function handleAuth(array $modes): void
continue;
}

$code = null;
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we still reset it on each iteration of the loop ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you are right. I added it in my forked branch should I create a new PR?

Copy link
Member

Choose a reason for hiding this comment

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

Please do :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

8000

Here it is: #51581 :)

@kgnblg kgnblg mentioned this pull request Sep 6, 2023
Merged
fabpot added a commit that referenced this pull request Sep 7, 2023
@fabpot
minor #51581 [Mailer] bug - reset $code variable each iteration for p…
95153c4 
…revent wrong exception codes (kgnblg)

This PR was merged into the 6.3 branch.

Discussion
----------

[Mailer] bug - reset $code variable each iteration for prevent wrong exception codes

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | ---
| License       | MIT
| Doc PR        | ---

Follow up PR of #51568 to reset `$code` variable for each iteration to have correct `TransportException` code.

(Note: This bug does not exist in version 5.4)

Commits
-------

ce4bc8c [mailer] reset $code variable each iteration for prevent wrong codes to be used
@fabpot fabpot mentioned this pull request Sep 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@xabbuh xabbuh xabbuh approved these changes

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
Bug Mailer Status: Reviewed
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.
5 participants
@kgnblg @carsonbot @fabpot @stof @xabbuh
0