Skip to content
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

testcase for broken Header\AbstractAddressList::fromString #44

Open
michalbundyra opened this issue Jan 15, 2020 · 7 comments
Open

testcase for broken Header\AbstractAddressList::fromString #44

michalbundyra opened this issue Jan 15, 2020 · 7 comments
Labels
Awaiting Author Updates Bug Something isn't working

Comments

@michalbundyra
Copy link
Member

Problem:

  1. GenericHeader loads the header in file, decodes it to utf-8
  2. the Headers::get attempts to Lazy-Load "To" header class

Lazyload does stringify and load in from string

$encoding = $current->getEncoding();
$headers  = $class::fromString($current->toString());

However, toString does not encode comma
AND To header class does split on comma!

see \Zend\Mail\Header\AbstractAddressList::fromString

this PR shows only the problem.


Originally posted by @glensc at zendframework/zend-mail#146

@michalbundyra
Copy link
Member Author

toString invocation path:


Originally posted by @glensc at zendframework/zend-mail#146 (comment)

@michalbundyra
Copy link
Member Author

perhaps the fix is in zend-mime project:
zendframework/zend-mime#26

if it gets accepted!


Originally posted by @glensc at zendframework/zend-mail#146 (comment)

@michalbundyra
Copy link
Member Author

after zendframework/zend-mime#26 being applied,
such code with Zend\Mail:

$headerLine = 'To: "=?iso-8859-1?Q?W=2C_bj=F8rn?=" <user@example.org>';
echo Mail\Header\GenericHeader::fromString($headerLine)->toString();

before:

To: =?UTF-8?Q?"W,=20bj=C3=B8rn"=20<user@example.org>?=

now:

To: =?UTF-8?Q?"W=2C=20bj=C3=B8rn"=20<user@example.org>?=

and

// with headerline without comma:
$headerLine = 'To: =?UTF-8?Q?"W=2C=20bj=C3=B8rn"=20<user@example.org>?=';
echo Mail\Header\To::fromString($headerLine)->toString();
To: =?UTF-8?Q?"W=2C=20bj=C3=B8rn"?= <user@example.org>

Originally posted by @glensc at zendframework/zend-mail#146 (comment)

@michalbundyra
Copy link
Member Author

@weierophinney closed this in zendframework/zend-mime@73e6d05 15 days ago

this is not correct. reopen please.


Originally posted by @glensc at zendframework/zend-mail#146 (comment)

@michalbundyra
Copy link
Member Author

@glensc I think I'm not understanding something.

The original string includes an encoded comma (=2c).

Why should the comma NOT be encoded when the To header is cast to a string? Is it because of the different encodings (iso-8859-1 vs UTF-8)?

(I need to understand why your expectation should be the expected behavior, basically.)


Originally posted by @weierophinney at zendframework/zend-mail#146 (comment)

@michalbundyra
Copy link
Member Author

@weierophinney it's combination of these two statements from Pull Description:

  • However, toString does not encode comma
  • AND To header class does split on comma!

i tried to explain the problem in test comments as well: https://github.com/zendframework/zend-mail/pull/146/files

if my explanation is not understandable (not sure i catched your question), just see the test code and how it behaves.

it's a lot of information and i dealed with this problem more than year ago....

so, this seemed the simplest way to solve the problem. it's not forbidden to zealously encode

as zendframework/zend-mime@73e6d05 was accepted in zendframework/zend-mime#26

this just updates unit test data.


Originally posted by @glensc at zendframework/zend-mail#146 (comment)

@michalbundyra
Copy link
Member Author

This repository has been moved to laminas/laminas-mail. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-mail to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-mail.
  • In your clone of laminas/laminas-mail, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

Originally posted by @weierophinney at zendframework/zend-mail#146 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Author Updates Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant