Skip to content

Prevent quoted_printable_encode from corrupting multibyte characters by splitting them between lines - #120

Closed
c2h5oh wants to merge 1 commit into
php:masterfrom
c2h5oh:master
Closed

c2h5oh wants to merge 1 commit into
php:masterfrom
c2h5oh:master

Conversation

@c2h5oh

@c2h5oh c2h5oh commented Jul 2, 2012

Copy link
Copy Markdown

Fixes https://bugs.php.net/bug.php?id=62462

quoted_printable_encode ignores multibyte characters when adding soft line breaks. If such break is inserted in the middle of multibyte char it corrupts the encoded string - this pull request fixes that.

@cjbj

cjbj commented Jul 2, 2012

Copy link
Copy Markdown
Contributor

Can you update this request and add some phpt tests?

@c2h5oh

c2h5oh commented Jul 2, 2012

Copy link
Copy Markdown
Author

Sure, but it will most likely have to wait till the weekend.

@c2h5oh

c2h5oh commented Jul 10, 2012

Copy link
Copy Markdown
Author

OK, quick question: what to do to match against string() if the original string contains \r\n? I did some testing and apparently even this fails:

--FILE--
<?php
print("a\r\nb");
?>
--EXPECT--
string(4) "a\r\nb"

Since I've to to check if \r\n is inserted in the right spot this is essential to get the tests working like that and not match lines separately.

@nikic

nikic commented Jul 10, 2012

Copy link
Copy Markdown
Member

Wouldn't you just test for

string(4) "a
b"

?

@c2h5oh

c2h5oh commented Jul 10, 2012

Copy link
Copy Markdown
Author

As I explained in the 2nd part of the comment: the whole bug is \r\n being inserted in the middle of multibyte character. I'd rather not have any control characters in other form than \n \t \r etc in the test to be sure the test is correct - it's easy to go wrong with non printable characters now or at any of the future edits.

@c2h5oh

c2h5oh commented Jul 10, 2012

Copy link
Copy Markdown
Author

OK, I have asked on php-qa and hopefully I'll get some info soon.

@c2h5oh

c2h5oh commented Jul 12, 2012

Copy link
Copy Markdown
Author

Two days later I still have no information how to properly write tests for this particular type of bug/fix - no answer on QA list ( http://news.php.net/php.qa/66659 ) and I can't find any docs that could help me here (phpt docs are too general).

Can anyone help me / point me in the right direction? I'd love to push this further.

@sstok

sstok commented Jul 15, 2012

Copy link
Copy Markdown

https://github.com/php/php-src/blob/master/ext/standard/tests/general_functions/002.phpt

https://github.com/php/php-src/blob/master/run-tests.php#L1978
Apparently CRLF is converted to just LF.

Is it possible to fool the tester by using?

--FILE--
<?php
print("a\r\nb");
?>
--EXPECT--
string(4) "a\r"."\nb"

Got it!

--FILE--
<?php
print("a\r"."\nb");
?>
--EXPECTREGEX--
a(\r)(\n)b

Because the input/regex is grouped (separate) it will not be replaced.

@c2h5oh

c2h5oh commented Jul 15, 2012

Copy link
Copy Markdown
Author
--TEST--
Multibyte characters shouldn't be split by soft line break added by quoted_printable_encode - 4 byte character test
--FILE--
<?php
echo quoted_printable_encode("01234567890123456789012345678901234567890123456789012345678901234567890123\xf0\x9f\x84\x90");
?>
--EXPECTREGEX--
01234567890123456789012345678901234567890123456789012345678901234567890123=(\r)(\n)=F0=9F=84=90
---- EXPECTED OUTPUT
01234567890123456789012345678901234567890123456789012345678901234567890123=(\r)(\n)=F0=9F=84=90
---- ACTUAL OUTPUT
01234567890123456789012345678901234567890123456789012345678901234567890123=
=F0=9F=84=90
---- FAILED

also \r is not present in .out file

@sstok

sstok commented Jul 16, 2012

Copy link
Copy Markdown

Hmm thats strange, why would anyone strip the \r of the actual result, thats just wrong.
I think we should just stick with \n until the tester is fixed (config flag for the test?).
Line ending for MIME is always \r\n even when using \n as user-input, in case of Swiftmailer. So its not really problem, just a formality.

@lstrojny

lstrojny commented Aug 8, 2012

Copy link
Copy Markdown
Contributor

@c2h5oh yep, stick with the replaced newlines. It doesn’t change the value of the test that much.

@php-pulls

Copy link
Copy Markdown

Comment on behalf of lstrojny at php.net:

PR merged and test added in 5.4 and master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants