Skip to content

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

Merged

Conversation

BogdanUngureanu
Copy link
Contributor

@BogdanUngureanu BogdanUngureanu commented Jun 16, 2025

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

@BogdanUngureanu BogdanUngureanu force-pushed the fix-listformatter-for-lower-icu branch from c9d99fc to e4dc2d9 Compare June 17, 2025 21:52
@BogdanUngureanu BogdanUngureanu changed the title WIP - Intl: Fix compile issues with ICU versions lower than 67 Intl: Fix compile issues with ICU versions lower than 67 Jun 17, 2025
@BogdanUngureanu
Copy link
Contributor Author

@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"
Copy link
Member

@devnexen devnexen Jun 17, 2025

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 ?

Copy link
Contributor Author

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'

@devnexen devnexen merged commit 5f67bac into php:master Jun 18, 2025
9 checks passed
@devnexen
Copy link
Member

Thanks for fixing it promptly !

@nielsdos
Copy link
Member

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.

@BogdanUngureanu
Copy link
Contributor Author

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:

  • have different arguments for the constructor depending on the ICU version - that would make it confusing and inconsistent
  • have the same signature for it, but don't have any error validation if they pass invalid data (e.g. TYPE_OR or a random value) - that would not inform the consumers that the formatting won't work as they expect it will work, it will always format with AND and WIDE.

@nielsdos
Copy link
Member

Okay I agree

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.

master fails to compile with icu 63
3 participants