Skip to content

Fix auto_detect_line_endings type #4700

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

Conversation

bugreportuser
Copy link
Contributor

Setting auto_detect_line_endings to on with ini_set doesn't enable it, because the value's parsed as a long.

Copy link
Member

@KalleZ KalleZ left a comment

Choose a reason for hiding this comment

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

Nice catch!

@cmb69
Copy link
Member

cmb69 commented Sep 13, 2019

Since this bugfix could break some setups, I think there should be an accompanying bug report to which we can refer in the changelog.

@bugreportuser
Copy link
Contributor Author

@cmb69
Copy link
Member

cmb69 commented Sep 14, 2019

Thanks! Applied as fdcca93.

@cmb69 cmb69 closed this Sep 14, 2019
@KalleZ
Copy link
Member

KalleZ commented Sep 14, 2019

@cmb69 wouldn't this be an ABI break for 7.2?

@bugreportuser bugreportuser deleted the fix-auto-detect-line-endings-type branch September 14, 2019 20:13
@cmb69
Copy link
Member

cmb69 commented Sep 14, 2019

Oh, indeed! So revert and merge into master only?

@KalleZ
Copy link
Member

KalleZ commented Sep 14, 2019

@cmb69 7.4+ should be fine tbh, its just a normal bug fix, rather we fix it for 7.4 than waiting another year

@nikic
Copy link
Member

nikic commented Sep 14, 2019

Leaving the type as zend_long and using OnUpdateBool should work fine for 7.2. (It may not write a proper boolean depending on endianness, but that doesn't matter so long as the value is non-zero.)

php-pulls pushed a commit that referenced this pull request Sep 15, 2019
@cmb69
Copy link
Member

cmb69 commented Sep 15, 2019

Thanks @KalleZ and @nikic! I have switched back to zend_long for PHP 7.2/7.3 (81cefab) but kept it for PHP 7.4+.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants