Skip to content

Conversation

@samsonasik
Copy link
Member

Q A
Bugfix yes

Description

Re-create zendframework/zend-mail#227 by @tzmfreedom Fixes #17 . Original PR description :

Zend\Mail\Transport\SendMail always append envelope-from parameter.
But I want to set any envelope-from.
In this PR, only when not set option -f, set -f option automatically.
This means if I set [email protected] or -bs [email protected], Zend\Mail\Transport\Sendmail doesn't set -f option.

Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

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

I've made one suggestion to how you check for the -f flag.

Additionally, we need tests demonstrating that the setParameters() method uses the parameter passed to the transport and NOT the sender and/or from address when it is provided; currently, you only demonstrate it is used when provided, and not used when not present, but not the additional case where it is not provided and falls through to the sender or from address.

weierophinney added a commit to samsonasik/laminas-mail that referenced this pull request Aug 6, 2020
Signed-off-by: Matthew Weier O'Phinney <[email protected]>
Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

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

I've combined the two unit tests into a single one backed by a data provider. I think it's ready to 🚢 once tests have run.

samsonasik and others added 4 commits August 6, 2020 09:24
This patch combines what was previously two test methods,
`testNotSetOptionAutomaticallyOnLeadingF` and
`testNotSetOptionAutomaticallyOnMiddleF`, into one method that better
describes the behaviour (`testDoesNotInjectFromParameterFromSenderWhenFromOptionPresentInParameters`)
and which uses a data provider to demonstrate various permutations of
parameters containing a `-f` switch.

Signed-off-by: Matthew Weier O'Phinney <[email protected]>
Signed-off-by: Matthew Weier O'Phinney <[email protected]>
@weierophinney weierophinney added this to the 2.12.2 milestone Aug 6, 2020
@weierophinney weierophinney added the Bug Something isn't working label Aug 6, 2020
@weierophinney weierophinney merged commit 108af31 into laminas:2.12.x Aug 6, 2020
@samsonasik samsonasik deleted the fix-17 branch August 6, 2020 14:29
weierophinney added a commit that referenced this pull request Aug 6, 2020
Signed-off-by: Matthew Weier O'Phinney <[email protected]>
artemii-karkusha pushed a commit to artemii-karkusha/laminas-mail that referenced this pull request May 24, 2023
Signed-off-by: Matthew Weier O'Phinney <[email protected]>
artemii-karkusha pushed a commit to artemii-karkusha/laminas-mail that referenced this pull request May 24, 2023
Signed-off-by: Matthew Weier O'Phinney <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants