[2.14]: Fix: Fold long lines during SMTP communication

[glensc]

Q	A
Documentation	no
Bugfix	yes
BC Break	no
New Feature	no
RFC	no
QA	yes

Description

It seems, the underlying problem of the bug is that currently, the loop reads up to 1000 bytes, and if the input is longer than that, then everything over 1000 bytes will appear as the next line in mail headers, resulting in corruption. And depending on MTA, such corruption is treated as the end of headers input or adds another mail header.

Updates #138 by @vladsf with the solution described in #138 (comment) and adds unit test.

[glensc]

I would move $chunkReader function to a new class (preferred) or to this class method. But really need some guidance from The Maintainers.

[vladsf]

I honestly believe that too much PHP force has been used :)

[vladsf]

I would move $chunkReader function to a new class (preferred) or to this class method. But really need some guidance from The Maintainers.

That was an impressive technique :) Wouldn't be simpler to move "escaping, rtrim'ing and folding" a line into a function?

[glensc]

That was an impressive technique :) Wouldn't be simpler to move "escaping, rtrim'ing and folding" a line into a function?

what do you mean by "simpler", simpler to whom? to what? i offered to move the generator to class method, but not doing 'jus for that', as maybe maintainers accept it as is now.

also, I would be against putting three different logics to the same code block, it would make code more difficult to understand and follow the code flow. currently, it's distinct, reading and unfolding in one place, the rest in the other loop. the code gets smelly if they all done in the same loop (as you can see from commit history). need multiple state variables that are unrelated to each other but used in the same loop.

[vladsf]

what do you mean by "simpler", simpler to whom? to what? i offered to move the generator to class method, but not doing 'jus for that', as maybe maintainers accept it as is now.

For the maintainers to review the PR.

[glensc]

what do you mean by "simpler", simpler to whom? to what? I offered to move the generator to class method, but not doing 'jus for that', as maybe maintainers accept it as is now.

For the maintainers to review the PR.

In the end, the resulting code is simpler, and only the aggregated diff is messy, but individual commit diffs are okay.

[glensc]

Created reproducer of the bug I discovered that prevented of making more reasonable tests:

• Test showing long wrapped lines are decoded incorrecty #142

Wanted to test like this:

        $raw = substr($data, strpos($data, "DATA\r\n") + 6);

        $message = new StorageMessage(['raw' => $raw]);
        $headers = $message->getHeaders()->toArray();
        $this->assertEquals($bufferSizeHeaderValue, $headers['X-Exact-Length']);
        $this->assertEquals($headerValue, $headers['X-Ms-Exchange-Antispam-Messagedata']);

That's what I get when captured SMTP from this PR. It does look correct, so it's a bug in parsing of this library/

[glensc]

@Slamdunk made a fresh rebase against upstream/2.14.x, perhaps it's better to review knowing it's up to date with the base branch.

[Slamdunk]

Thank you @glensc, I'm going to squash this PR to reduce git-blame and git-bisect verbosity

[glensc]

@Slamdunk perhaps you should have asked first from authors for permission to squash!

This PR has waited for two weeks+ being idle! If this was an issue, should have said it in that time period!

I (try to) follow the atomic commits principle:

• https://www.freshconsulting.com/atomic-commits/

As I see it, you made the opposite: more difficult to git blame/bisect with squashing. And losing commit author information is not very friendly either.

[glensc]

Update: seems this got merged without squashing, at least the 542686a merge commit. So far so good.

[glensc]

@Slamdunk thank you for the 2.14.0 release!

[glensc]

Seems the release automation can't handle that PR had multiple contributors:

• https://github.com/laminas/laminas-mail/releases/tag/2.14.0

140: [2.14]: Fix: Fold long lines during SMTP communication thanks to @glensc

it actually replaced #138 by @vladsf yet there's no credit about that.