ext/intl: Reject unconstructed Collator in attribute and strength methods

[iliaal]

Collator::getAttribute(), setAttribute(), getStrength() and setStrength() reach ICU with a NULL collator when a subclass skips parent::__construct(), so getAttribute() returns -1 and setStrength() reports success on an object that cannot collate. The four methods that already check, compare(), getLocale(), sort() and getSortKey(), throw "Object not initialized"; this applies the same guard to the remaining four. COLLATOR_METHOD_FETCH_OBJECT does not check on its own.

[LamentXU123]

Is it possible to abstract these stuff to a shared helper?

[LamentXU123]

i.e.

static zend_always_inline zend_result collator_check_initialized(
	Collator_object *co)
{
	ZEND_ASSERT(co != NULL);

	if (UNEXPECTED(co->ucoll == NULL)) {
		intl_error_set_code(NULL, COLLATOR_ERROR_CODE(co));
		intl_errors_set_custom_msg(
			COLLATOR_ERROR_P(co), "Object not initialized", 0);
		zend_throw_error(NULL, "Object not initialized");
		return FAILURE;
	}

	return SUCCESS;
}

[iliaal]

Added collator_check_initialized() to collator_class.h and routed the four guards through it. Kept your ZEND_ASSERT: co comes from Z_INTL_COLLATOR_P(), which is offset arithmetic on an already-validated zend_object, so the !co half I had was never reachable.

Left the pre-existing guards in collator_compare.c, collator_locale.c and collator_sort.c alone for now. The one in collator_sort.c uses a shorter shape that throws without setting the intl error state, so folding those in would change observable error state rather than just dedupe.

[LamentXU123]

Several small suggestions.
a. Can we utilize this helper in other functions? compare()? getLocale()?
b. Could you also extend the test to cover collator_get_attribute(), collator_set_attribute(), collator_get_strength(), and collator_set_strength() with an unconstructed Collator?

[iliaal]

Done for compare(), getLocale(), sortWithSortKeys() and getSortKey(). Left the one in collator_sort_internal() (sort() and asort()): it only throws, where the helper also sets the object's intl error and, under intl.error_level, emits a warning. On an unconstructed Collator today, collator_get_error_message() returns "U_ZERO_ERROR" after a caught sort() against "Object not initialized: U_ZERO_ERROR" after compare(). That's a behavior change rather than a dedupe, so I'd keep it separate.

Test now covers collator_get_attribute(), collator_set_attribute(), collator_get_strength() and collator_set_strength() alongside the methods.

[devnexen]

can collator_sort_internal() be taking care of while at it ?

[iliaal]

Done in 014133d. One behavior change to note: the helper also sets the intl error state, so sort() and asort() now emit the diagnostic under a non-default intl.error_level, and at E_ERROR that turns a catchable Error into a fatal. compare(), getLocale(), getSortKey() and sortWithSortKeys() already do that on 8.4, so this closes a gap rather than opening one, but it is still a behavior change on a stable branch. Under the default settings the only difference is collator_get_error_message() returning "Object not initialized: U_ZERO_ERROR" instead of "U_ZERO_ERROR".

If you'd rather 8.4 kept to the four attribute methods and the sort_internal() conversion went to master, I'll split it (I think that is best).

[devnexen]

indeed