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

Fix quote header regexes #57

Merged
merged 2 commits into from
May 22, 2017

Conversation

kipit
Copy link
Contributor

@kipit kipit commented May 17, 2017

Hello,

This PR fix the gluttony of some quote header regexes. ATM, the change has only been applied to English, French, Spanish and Italian headers.

@kipit
Copy link
Contributor Author

kipit commented May 19, 2017

Hi @willdurand,

We have added a test case which lead to a segfault in php-5.6 with the old quote header regex.
I’m still struggling with the Travis build… Right now, in this PR at least, HHVM build don't pass.

I've try some Travis variation in another PR #58 but without any success.

I'm stuck at that point, if someone has any idea, let me know ;-)

@willdurand
Copy link
Owner

Let's fix Travis-CI setup first, then we'll look into this PR. I am working on it :)

@willdurand willdurand force-pushed the improve_quote_header_handling branch from ce2c386 to 8273573 Compare May 22, 2017 12:49
@willdurand
Copy link
Owner

@kipit sorry for having pushed into your own fork, but it looks like everything is back to stable, right? WDYT? Should I merge? Travis-CI does not get any fault in 5.6.

@kipit kipit force-pushed the improve_quote_header_handling branch from 8273573 to 460c717 Compare May 22, 2017 22:16
@kipit
Copy link
Contributor Author

kipit commented May 22, 2017

I've rebased my branch to your master, and squashed my commits to ease the merge. All tests passed, so, from my point of view, you can merge.

FYI: the segfault occure, only on php5, if you change the spanish regex from:

/^\s*(El(?:(?!^>*\s*El\b|\bescribió:).){0,1000}escribió:)$/ms

to

/^\s*(El(?:(?!^>*\s*El\b|\bescribió:).)*escribió:)$/ms

Finally, this is a small improvement cause, 1000 should be sufficient to grab a quote header on multiple lines.

@willdurand willdurand merged commit 3a73e35 into willdurand:master May 22, 2017
@willdurand
Copy link
Owner

perfect, thanks!

@kipit kipit deleted the improve_quote_header_handling branch May 22, 2017 22:39
turt2live added a commit to t2bot/node-email-reply-parser that referenced this pull request Jul 4, 2017
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.

2 participants