Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

Fix issue #22 #23

Closed
wants to merge 2 commits into from
Closed

Fix issue #22 #23

wants to merge 2 commits into from

Conversation

matbech
Copy link

@matbech matbech commented Jul 29, 2015

Remove the subject header otherwise it will be duplicated.

Remove the subject header otherwise it will be duplicated.
@mwillbanks
Copy link
Contributor

@matbech can you please add tests for this?

@matbech
Copy link
Author

matbech commented Sep 13, 2015

The Subject header is added by the php mail function on Windows. If an extra Subject header is present in the header array the result is a message with two Subject headers.

The request for an automated test is not reasonable because:
1.There is already enough evidence of the bug:

2.Writing an STMP server to intercept the mail communication from Window's php:mail is unreasonable, considering the evidence already presented.

@glensc
Copy link
Contributor

glensc commented Feb 9, 2016

@matbech can you just make test that if OS is windows the header is removed? not sure if the isWindows result can be mocked.

as for the proof, maybe it's behavior change in php 7, and behaving differently in php < 7

@matbech
Copy link
Author

matbech commented Feb 18, 2016

I'm sorry but I have a hard time to understand your sentence. This has been tested on Windows with PHP 7. There is no good reason to leave this bug open for more than 6 months now.

@glensc
Copy link
Contributor

glensc commented Feb 18, 2016

Did you test all other supported versions too?, PHP < 7

I'm trying to help, i don't care for one bit for windows support personally!
as it's hard to understand how the original code landed, it probably behaved differently back then, or different windows, the other option would be the code was committed without any testing, i just find that hard to believe!

@weierophinney
Copy link
Member

We've asked for tests, and blocked this for lack of tests; it's not a question of whether or not we want the fix, but wanting to ensure it stays fixed.

You can test this by mocking the result of what isWindowsOs() returns (potentially by extending the transport to hardcode the value), and checking the message generated to see if the header was removed.

If/when you are able to do that, please open a new pull request; until then, I'm closing this for now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants