ext/intl: Guard Spoofchecker restriction-level APIs at ICU 53 (fix build on ICU 50.x)#22248
ext/intl: Guard Spoofchecker restriction-level APIs at ICU 53 (fix build on ICU 50.x)#22248GrahamCampbell wants to merge 2 commits into
Conversation
233e97c to
f814198
Compare
f814198 to
384a146
Compare
LamentXU123
left a comment
There was a problem hiding this comment.
Indeed correct analysis. I wasn't aware PHP 8.4 has a different minimum ICU version than 8.5 and 8.6. Thank you for this PR!
|
@GrahamCampbell We may need to add this into the PHP 8.4 NEWS file to notice others that we fix this. |
|
Added a news entry. |
|
@GrahamCampbell Thank you. The PR looks good and we will merge it if @devnexen has no opinions :) |
|
none I just wanted to see how you do :) |
| public const int INVISIBLE = UNKNOWN; | ||
| /** @cvalue USPOOF_CHAR_LIMIT */ | ||
| public const int CHAR_LIMIT = UNKNOWN; | ||
| #if U_ICU_VERSION_MAJOR_NUM >= 53 |
There was a problem hiding this comment.
Ideally, we may still want to support ICU 51 since it is technically >50.1, which is our minimum supported version in 8.4. So I'd suggest to only guard USPOOF_SINGLE_SCRIPT_RESTRICTIVE under ICU 53 and guard the rest under ICU 51. This benefit people who are using 51 < ICU version < 53 that they should have access to these const that exists since ICU 51
That is, I suggest to change this guard to >= 51 and specifically list USPOOF_SINGLE_SCRIPT_RESTRICTIVE out and put it in a ICU >= 53 guard.
| <?php | ||
| $r = new ReflectionClass("SpoofChecker"); | ||
| if (false === $r->getConstant("SINGLE_SCRIPT_RESTRICTIVE")) { | ||
| die("skip Incompatible ICU version"); |
There was a problem hiding this comment.
| die("skip Incompatible ICU version"); | |
| die("skip for ICU version < 53"); |
to align with other skip message in the intl extension. Might need to consider to change the version message to 51 if the upper suggestion is agreed.
| <?php | ||
| $r = new ReflectionClass("Spoofchecker"); | ||
| if (false === $r->getConstant("SINGLE_SCRIPT_RESTRICTIVE")) { | ||
| die("skip Incompatible ICU version"); |
There was a problem hiding this comment.
| die("skip Incompatible ICU version"); | |
| die("skip for ICU version < 53"); |
Ditto.
| <?php | ||
| if (!class_exists("Spoofchecker")) print 'skip'; | ||
|
|
||
| if (!method_exists(new Spoofchecker(), 'setRestrictionLevel')) print 'skip ICU version < 53'; |
There was a problem hiding this comment.
| if (!method_exists(new Spoofchecker(), 'setRestrictionLevel')) print 'skip ICU version < 53'; | |
| if (!method_exists(new Spoofchecker(), 'setRestrictionLevel')) print 'skip for ICU version < 53'; |
Ditto. However this can always be accepted whether we agree to move the majority of your guard to 51 because the setRestrictionLevel is guarded under 53 anyways.
PHP 8.4.22 fails to compile the intl extension when the system ICU is older than 53 — on Amazon Linux 2 (system ICU 50.2) the build breaks with
USPOOF_ASCII, the otherSpoofcheckerrestriction-level constants, andURestrictionLevelundeclared. The cause is GH-22055 (fixing #22053), which removed the#if U_ICU_VERSION_MAJOR_NUM >= 58guards around theSpoofcheckerrestriction-level constants and thesetRestrictionLevel()method, so they are now referenced unconditionally.This is fine on master and PHP-8.5 (
icu-uc >= 57.1) but not on PHP-8.4, which still acceptsicu-uc >= 50.1. The symbols have existed since ICU 51 exceptUSPOOF_SINGLE_SCRIPT_RESTRICTIVE(ICU 53, used in thesetRestrictionLevel()body); since 50.2 satisfies the50.1minimum,configurepasses and only the compile fails. ICU 53 is therefore the right floor: guarding at>= 53builds on every ICU PHP-8.4 supports while still exposing the API on ICU 53+ — which is what GH-22055 wanted, since its>= 58guard was too strict and hid the API on ICU 57.x.The fix restores the guard at
#if U_ICU_VERSION_MAJOR_NUM >= 53around the constants andsetRestrictionLevel()inspoofchecker.stub.php(regeneratingspoofchecker_arginfo.h) and around thesetRestrictionLevel()body inspoofchecker_main.c. The unrelatedsetAllowedChars()message change is kept.