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

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

Closed
wants to merge 1 commit into from

Conversation

c2h5oh
Copy link

@c2h5oh c2h5oh commented Jul 2, 2012

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
Copy link
Contributor

cjbj commented Jul 2, 2012

Can you update this request and add some phpt tests?

@c2h5oh
Copy link
Author

c2h5oh commented Jul 2, 2012

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

@c2h5oh
Copy link
Author

c2h5oh commented Jul 10, 2012

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
Copy link
Member

nikic commented Jul 10, 2012

Wouldn't you just test for

string(4) "a
b"

?

@c2h5oh
Copy link
Author

c2h5oh commented Jul 10, 2012

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
Copy link
Author

c2h5oh commented Jul 10, 2012

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

@c2h5oh
Copy link
Author

c2h5oh commented Jul 12, 2012

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
Copy link

sstok commented Jul 15, 2012

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
Copy link
Author

c2h5oh commented Jul 15, 2012

--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
Copy link

sstok commented Jul 16, 2012

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
Copy link
Contributor

lstrojny commented Aug 8, 2012

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

@php-pulls
Copy link

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