Declare Transliterator::$id as readonly to unlock subclassing it

[nicolas-grekas]

Before PHP 8.2 it was possible to create a child translator with a custom $id by using unserialize():

unserialize(sprintf('O:%d:"%s":1:{s:2:"id";s:%d:"%s";}', \strlen(self::class), self::class, \strlen($id), $id))

But since #7945, this doesn't work anymore, causing a BC break with no alternative.

Yet, there is a way to make this work in 8.2, which is to remove the custom code that makes $id read-only and replacing it with a native readonly declaration.

So here we are, see test case also.

[cmb69]

Transliterator should better have been final, but that ship has sailed, so this looks like a good idea to mitigate the BC break.

@kocsismate, thoughts?

[nicolas-grekas]

@cmb69 thanks for the review. Could you please help me on the failure?
make test TESTS=ext/intl/tests/transliterator_clone.phpt fails with Cannot modify readonly property Transliterator::$id.
I guess that's because Transliterator_clone_obj calls zend_objects_clone_members( &to_new->zo, &to_orig->zo ); but I'm not sure how the code should be instead...

[cmb69]

The problem:

php-src/ext/intl/transliterator/transliterator_class.c

Lines 143 to 157 in 520bb2e

	zend_objects_clone_members( &to_new->zo, &to_orig->zo );
	if( to_orig->utrans != NULL )
	{
	zval tempz; /* dummy zval to pass to transliterator_object_construct */
	/* guaranteed to return NULL if it fails */
	UTransliterator *utrans = utrans_clone( to_orig->utrans, TRANSLITERATOR_ERROR_CODE_P( to_orig ) );
	if( U_FAILURE( TRANSLITERATOR_ERROR_CODE( to_orig ) ) )
	goto err;
	ZVAL_OBJ(&tempz, ret_val);
	transliterator_object_construct( &tempz, utrans,
	TRANSLITERATOR_ERROR_CODE_P( to_orig ) );

In the first line the properties are cloned (so id === "hex-any"), and in the last line the object is constructed, so that the id is set again. It would be sufficient to remove the zend_objects_clone_members() call, but since Transliterator supports dynamic properties 😢, we cannot for BC reasons.

I guess we can do something like:

 ext/intl/transliterator/transliterator_class.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ext/intl/transliterator/transliterator_class.c b/ext/intl/transliterator/transliterator_class.c
index 2dbc940f37..3610b03d84 100644
--- a/ext/intl/transliterator/transliterator_class.c
+++ b/ext/intl/transliterator/transliterator_class.c
@@ -152,9 +152,7 @@ static zend_object *Transliterator_clone_obj( zend_object *object )
 		if( U_FAILURE( TRANSLITERATOR_ERROR_CODE( to_orig ) ) )
 			goto err;
 
-		ZVAL_OBJ(&tempz, ret_val);
-		transliterator_object_construct( &tempz, utrans,
-			TRANSLITERATOR_ERROR_CODE_P( to_orig ) );
+		to_new->utrans = utrans;
 
 		if( U_FAILURE( TRANSLITERATOR_ERROR_CODE( to_orig ) ) )
 		{

(and do some cleanup regarding the error handling)

[nicolas-grekas]

+ to_new->utrans = utrans;

That worked thanks! (and that makes sense)

(and do some cleanup regarding the error handling)

not sure what you mean here :)

[codecov-commenter]

Codecov Report

Merging #9167 (520bb2e) into master (520bb2e) will not change coverage.
The diff coverage is n/a.

❗ Current head 520bb2e differs from pull request most recent head dcf0aed. Consider uploading reports for the commit dcf0aed to get more accurate results

@@           Coverage Diff           @@
##           master    #9167   +/-   ##
=======================================
  Coverage   67.12%   67.12%           
=======================================
  Files         841      841           
  Lines      311272   311272           
=======================================
  Hits       208936   208936           
  Misses     102336   102336           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[cmb69]

That looks right now. Thank you!

I'm leaving this open for a while, so that others can review.

[Girgias]

LGTM