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

Fix HeaderValue throwing an exception on legal characters#65

Closed
avasa wants to merge 4 commits intozendframework:masterfrom
avasa:master
Closed

Fix HeaderValue throwing an exception on legal characters#65
avasa wants to merge 4 commits intozendframework:masterfrom
avasa:master

Conversation

@avasa
Copy link

@avasa avasa commented Feb 10, 2016

Original pull request #24 created Oct 7 2015.

Author of pull request has not responded since Nov 4 2015.

Can we please get this merged and resolved.

Thanks.

@Maks3w
Copy link
Member

Maks3w commented Feb 10, 2016

Need tests

@Maks3w Maks3w added the Test label Feb 10, 2016
@avasa
Copy link
Author

avasa commented Feb 24, 2016

Any updates on this?

["This\tis\ta test", 'assertTrue'],
["This is\ta \r\n test", 'assertTrue'],
["This\tis\ta\ntest", 'assertFalse'],
["This is a \r\t\n \r\n test", 'assertFalse'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test case for this value ["This is a\r\n\ttest", 'asserTrue'],

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't make it assert true. Breaks other the tests. Can you please take a look. Much appreciated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not admisible push a commit with a wrong assertion just for to make the test to pass.

We prefer a failure in the test before to ship buggy code.

Please change the assertion.

@Maks3w
Copy link
Member

Maks3w commented Mar 24, 2016

Now take a look to the line ~88 if ($lf !== 10 || $sp !== 32) { play with $sp var

@Maks3w Maks3w removed the Test label Apr 16, 2016
@weierophinney weierophinney mentioned this pull request May 5, 2016
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.

2 participants