-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Intl: Fix compile issues with ICU versions lower than 67 #18868
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
Intl: Fix compile issues with ICU versions lower than 67 #18868
Conversation
c9d99fc
to
e4dc2d9
Compare
@devnexen if you have some spare time, it would be great if you could have a look. |
#include "listformatter_class.h" | ||
#include "listformatter_arginfo.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks fine, did you need that change though ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that's needed because the stub contains references to the fallbacks. listformatter_arginfo.h needs to be after.
Otherwise it would fail with
php-src/ext/intl/listformatter/listformatter_arginfo.h:50:35: error: use of undeclared identifier 'INTL_LISTFORMATTER_FALLBACK_TYPE_AND'
Thanks for fixing it promptly ! |
Does this fix make sense? If you're on an older version the constants are set to 0. Normally, when functionality is not available, we don't expose the constant at all. Diverging from this is confusing imo. |
I think it does because it makes the public contract consistent for the constructor. Otherwise, consumers will not know what values the constructor accepts for type and width. If we didn't have the TYPE_AND and WIDE fallbacks, we would have 2 options:
|
Okay I agree |
fixes #18860
It gets built, but I'm having some environment issues with setting up ICU 63, so I haven't actually run the tests to see if it's working.When PHP intl is built with an older version of ICU (66 or older), we need a fallback for the AND and WIDE parameters. To solve this, I've added INTL_LISTFORMATTER_FALLBACK_TYPE_AND and INTL_LISTFORMATTER_FALLBACK_WIDTH_WIDE.
I've ran it locally with ICU 63 and the change works as expected (the tests pass). There's a failing test related to currencies, but it's unrelated to this change (some CLDR differences between versions).
cc @SjonHortensius