Skip to content

Conversation

alexandre-daubois
Copy link
Member

Fix #19188

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

It looks good. Just few minor things.

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

I went through it again few times and don't see any issue. All good though.

@php/release-managers-85 are you ok with this going to 8.5? It should hopefully resolve issues for some mail users.

Copy link
Member

@DanielEScherzer DanielEScherzer left a comment

Choose a reason for hiding this comment

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

Adding a new INI option seems like something that should have an internals discussion, if not a full RFC
But I'll leave the call to @edorian about if this is small enough to go into 8.5

Copy link
Member

@edorian edorian left a comment

Choose a reason for hiding this comment

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

Doesn't seem controversial, PR is open for a long time. Given that, I don't see much use on delaying this for a year.

So 👍 from me for 8.5

@alexandre-daubois alexandre-daubois merged commit ae7def7 into php:master Sep 8, 2025
9 checks passed
@alexandre-daubois alexandre-daubois deleted the gh-19188 branch September 8, 2025 07:58
Comment on lines +729 to +733
if (ZSTR_LEN(new_value) > 0 &&
strcmp(val, "crlf") != 0 &&
strcmp(val, "lf") != 0 &&
strcmp(val, "mixed") != 0 &&
strcmp(val, "os") != 0) {
Copy link
Member

Choose a reason for hiding this comment

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

This could've been zend_string_equals_literal() to use the higher-level zend_string APIs (making the code more readable).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes that's cleaner. Addressed in #19759

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mail: introduce new INI for line ending control

5 participants