Skip to content

WIP - Intl: Add a new IntlNumberRangeFormatter class #19232

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ext/intl/config.m4
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ if test "$PHP_INTL" != "no"; then
dateformat/datepatterngenerator_class.cpp \
dateformat/datepatterngenerator_methods.cpp \
msgformat/msgformat_helpers.cpp \
rangeformatter/rangeformatter_class.cpp \
timezone/timezone_class.cpp \
timezone/timezone_methods.cpp \
calendar/calendar_class.cpp \
Expand Down Expand Up @@ -123,6 +124,7 @@ if test "$PHP_INTL" != "no"; then
$ext_builddir/listformatter
$ext_builddir/msgformat
$ext_builddir/normalizer
$ext_builddir/rangeformatter
$ext_builddir/resourcebundle
$ext_builddir/spoofchecker
$ext_builddir/timezone
Expand Down
3 changes: 3 additions & 0 deletions ext/intl/config.w32
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ if (PHP_INTL != "no") {
normalizer_class.c \
normalizer_normalize.c \
", "intl");
ADD_SOURCES(configure_module_dirname + "/rangeformatter", "\
rangeformatter_class.cpp \
", "intl");
ADD_SOURCES(configure_module_dirname + "/dateformat", "\
dateformat.c \
dateformat_class.c \
Expand Down
4 changes: 4 additions & 0 deletions ext/intl/php_intl.c
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
#include "locale/locale_class.h"

#include "listformatter/listformatter_class.h"
#include "rangeformatter/rangeformatter_class.h"

#include "dateformat/dateformat.h"
#include "dateformat/dateformat_class.h"
Expand Down Expand Up @@ -161,6 +162,9 @@ PHP_MINIT_FUNCTION( intl )
/* Register 'ListFormatter' PHP class */
listformatter_register_class( );

/* Register 'NumberRangeFormatter' PHP class */
rangeformatter_register_class( );

/* Register 'Normalizer' PHP class */
normalizer_register_Normalizer_class( );

Expand Down
40 changes: 40 additions & 0 deletions ext/intl/rangeformatter/rangeformatter.stub.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?php

/** @generate-class-entries */

/**
* @not-serializable
* @strict-properties
*/
final class IntlNumberRangeFormatter {
Copy link
Member

Choose a reason for hiding this comment

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

you should have a condition around the class so that it's only exposed for libicu 68+. Similar example:

#if U_ICU_VERSION_MAJOR_NUM >= 67


/** @cvalue UNUM_RANGE_COLLAPSE_AUTO */
public const int COLLAPSE_AUTO = UNKNOWN;

/** @cvalue UNUM_RANGE_COLLAPSE_NONE */
public const int COLLAPSE_NONE = UNKNOWN;

/** @cvalue UNUM_RANGE_COLLAPSE_UNIT */
public const int COLLAPSE_UNIT = UNKNOWN;

/** @cvalue UNUM_RANGE_COLLAPSE_ALL */
public const int COLLAPSE_ALL = UNKNOWN;

/** @cvalue UNUM_IDENTITY_FALLBACK_SINGLE_VALUE */
public const int IDENTITY_FALLBACK_SINGLE_VALUE = UNKNOWN;

/** @cvalue UNUM_IDENTITY_FALLBACK_APPROXIMATELY_OR_SINGLE_VALUE */
public const int IDENTITY_FALLBACK_APPROXIMATELY_OR_SINGLE_VALUE = UNKNOWN;

/** @cvalue UNUM_IDENTITY_FALLBACK_APPROXIMATELY */
public const int IDENTITY_FALLBACK_APPROXIMATELY = UNKNOWN;

/** @cvalue UNUM_IDENTITY_FALLBACK_RANGE */
public const int IDENTITY_FALLBACK_RANGE = UNKNOWN;

private function __construct() {}

public static function createFromSkeleton(string $skeleton, string $locale, int $collapse, int $identityFallback): IntlNumberRangeFormatter {}
Copy link
Member

@kocsismate kocsismate Jul 25, 2025

Choose a reason for hiding this comment

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

I hope I won't upset you too much, but I think these changes will need an RFC, because any API changes involve a lot of bikeshedding lately.

For example, we have started to use enums more often (see my https://wiki.php.net/rfc/url_parsing_api RFC), so I think this practice could be continued in your case too.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I see that you have recently added the ListFormatter class in #18519 that also has some enum-like constants. It probably also makes sense to stay consistent with the current convention of intl (class constants), so I don't insist on my above suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

If we decide that we should employ nicer features in intl, we should do a thorough review of not only the constants ,but other mechanisms (like error handling) as well. And then we can make one compelling holistic RFC (if we would desire to do so ;) ).


public function format(float|int $start, float|int $end): string {}
}
Copy link
Member

@kocsismate kocsismate Jul 25, 2025

Choose a reason for hiding this comment

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

IntlListFormatter has the getError*() methods. Why didn't you implement them for IntlNumberRangeFormatter? (maybe that's what you still need to implement according to the last sentence of the PR description?)

86 changes: 86 additions & 0 deletions ext/intl/rangeformatter/rangeformatter_arginfo.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

180 changes: 180 additions & 0 deletions ext/intl/rangeformatter/rangeformatter_class.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,180 @@
/*
+----------------------------------------------------------------------+
| This source file is subject to version 3.01 of the PHP license, |
| that is bundled with this package in the file LICENSE, and is |
| available through the world-wide-web at the following url: |
| https://www.php.net/license/3_01.txt |
| If you did not receive a copy of the PHP license and are unable to |
| obtain it through the world-wide-web, please send a note to |
| license@php.net so we can mail you a copy immediately. |
+----------------------------------------------------------------------+
| Authors: Bogdan Ungureanu <bogdanungureanu21@gmail.com> |
+----------------------------------------------------------------------+
*/

#include <unicode/numberrangeformatter.h>
#include <unicode/unumberrangeformatter.h>
#include <unicode/numberformatter.h>
#include <unicode/unistr.h>
#include "../intl_convertcpp.h"

extern "C" {
#include "php.h"
#include "../php_intl.h"
#include "../intl_data.h"
#include "rangeformatter_arginfo.h"
#include "rangeformatter_class.h"
#include "intl_convert.h"
}

using icu::number::NumberRangeFormatter;
using icu::number::NumberFormatter;
using icu::number::UnlocalizedNumberFormatter;
using icu::number::LocalizedNumberRangeFormatter;
using icu::UnicodeString;
using icu::MeasureUnit;

static zend_object_handlers rangeformatter_handlers;
zend_class_entry *class_entry_IntlNumberRangeFormatter;

zend_object *IntlNumberRangeFormatter_object_create(zend_class_entry *ce)
{
IntlNumberRangeFormatter_object* intern;

intern = (IntlNumberRangeFormatter_object*)zend_object_alloc(sizeof(IntlNumberRangeFormatter_object), ce);
zend_object_std_init(&intern->zo, ce);
object_properties_init(&intern->zo, ce);

return &intern->zo;
}

U_CFUNC PHP_METHOD(IntlNumberRangeFormatter, __construct)
{

Comment on lines +52 to +53
Copy link
Member

Choose a reason for hiding this comment

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

I think you could throw an exception if this functions is tried to be called:

zend_throw_error(NULL, "Cannot directly construct %s, use document methods instead", ZSTR_VAL(Z_OBJCE_P(ZEND_THIS)->name));

Copy link
Member

@kocsismate kocsismate Jul 25, 2025

Choose a reason for hiding this comment

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

+ please add a test for this case

}

U_CFUNC PHP_METHOD(IntlNumberRangeFormatter, createFromSkeleton)
{
char* skeleton;
char* locale;
size_t locale_len;
size_t skeleton_len;
zend_long collapse;
zend_long identityFallback;

ZEND_PARSE_PARAMETERS_START(4,4)
Z_PARAM_STRING(skeleton, skeleton_len)
Z_PARAM_STRING(locale, locale_len)
Z_PARAM_LONG(collapse)
Z_PARAM_LONG(identityFallback)
ZEND_PARSE_PARAMETERS_END();

if (locale_len == 0) {
locale = (char *)intl_locale_get_default();
}

if (skeleton_len == 0) {
zend_argument_value_error(1, "Skeleton string cannot be empty");
Copy link
Member

@kocsismate kocsismate Jul 25, 2025

Choose a reason for hiding this comment

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

The error messages don't fit zend_argument_value_error. E.g. the result of this one will read as Argument #1 ($skeleton) Skeleton string cannot be empty. So in this case, you should change the 2nd argument to cannot be empty.

Copy link
Member

Choose a reason for hiding this comment

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

Better yet, use zend_argument_must_not_be_empty_error(1); which will ensure standard wording

RETURN_THROWS();
}

if (locale_len > INTL_MAX_LOCALE_LEN) {
zend_argument_value_error(2, "Locale string too long, should be no longer than %d characters", INTL_MAX_LOCALE_LEN);
RETURN_THROWS();
}

if (strlen(uloc_getISO3Language(locale)) == 0) {
zend_argument_value_error(2, "\"%s\" is invalid", locale);
RETURN_THROWS();
}

if (collapse != UNUM_RANGE_COLLAPSE_AUTO && collapse != UNUM_RANGE_COLLAPSE_NONE && collapse != UNUM_RANGE_COLLAPSE_UNIT && collapse != UNUM_RANGE_COLLAPSE_ALL) {
zend_argument_value_error(3, "Invalid collapse value");
RETURN_THROWS();
}

if (identityFallback != UNUM_IDENTITY_FALLBACK_SINGLE_VALUE && identityFallback != UNUM_IDENTITY_FALLBACK_APPROXIMATELY_OR_SINGLE_VALUE && identityFallback != UNUM_IDENTITY_FALLBACK_APPROXIMATELY && identityFallback != UNUM_IDENTITY_FALLBACK_RANGE) {
zend_argument_value_error(4, "Invalid identity fallback value");
RETURN_THROWS();
}

UErrorCode error = U_ZERO_ERROR;

UnicodeString skeleton_ustr(skeleton, skeleton_len);

UnlocalizedNumberFormatter nf = NumberFormatter::forSkeleton(skeleton_ustr, error);

LocalizedNumberRangeFormatter* nrf = new LocalizedNumberRangeFormatter(
NumberRangeFormatter::with()
.locale(locale)
.numberFormatterBoth(nf)
.collapse(static_cast<UNumberRangeCollapse>(collapse))
.identityFallback(static_cast<UNumberRangeIdentityFallback>(identityFallback))
);

zend_object* obj = IntlNumberRangeFormatter_object_create(class_entry_IntlNumberRangeFormatter);
obj->handlers = &rangeformatter_handlers;

RANGEFORMATTER_OBJECT(php_intl_numberrangeformatter_fetch_object(obj)) = nrf;

RETURN_OBJ(obj);

}

U_CFUNC PHP_METHOD(IntlNumberRangeFormatter, format)
{
double start;
double end;

IntlNumberRangeFormatter_object* obj = Z_INTL_RANGEFORMATTER_P(ZEND_THIS);

ZEND_PARSE_PARAMETERS_START(2, 2)
Z_PARAM_DOUBLE(start)
Copy link
Member

Choose a reason for hiding this comment

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

this macro implies float type, but you would want int|float which needs Z_PARAM_NUMBER

Z_PARAM_DOUBLE(end)
ZEND_PARSE_PARAMETERS_END();

UErrorCode error = U_ZERO_ERROR;

UnicodeString result = RANGEFORMATTER_OBJECT(obj)->formatFormattableRange(start, end, error).toString(error);

if (U_FAILURE(error)) {
intl_error_set(NULL, error, "Failed to format number range", 0);

return;
}

zend_string *ret = intl_charFromString(result, &error);

if (!ret) {
intl_error_set(NULL, error, "Failed to convert result to UTF-8", 0);
// RETVAL_FALSE;
return;
Copy link
Member

Choose a reason for hiding this comment

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

this (and the return above) will result in a null return value which is missing from the stub, so it should either be added there, or you should throw an exception.

}

RETVAL_NEW_STR(ret);
}

void IntlNumberRangeFormatter_object_free(zend_object *object)
{
IntlNumberRangeFormatter_object* nfo = php_intl_numberrangeformatter_fetch_object(object);

if (nfo->nrf_data.unumrf) {
delete nfo->nrf_data.unumrf;
nfo->nrf_data.unumrf = nullptr;
}

intl_error_reset(&nfo->nrf_data.error);

zend_object_std_dtor(&nfo->zo);
}

void rangeformatter_register_class(void)
{
class_entry_IntlNumberRangeFormatter = register_class_IntlNumberRangeFormatter();
class_entry_IntlNumberRangeFormatter->create_object = IntlNumberRangeFormatter_object_create;

memcpy(&rangeformatter_handlers, zend_get_std_object_handlers(), sizeof(zend_object_handlers));
rangeformatter_handlers.offset = XtOffsetOf(IntlNumberRangeFormatter_object, zo);
rangeformatter_handlers.free_obj = IntlNumberRangeFormatter_object_free;
rangeformatter_handlers.clone_obj = NULL;
}
Loading
Loading