Compatibility with PHP 8.2 #102
Conversation
|
It looks some others are missing (username / password) (the reason why I go to the trivial way in pr #104) |
…mcache->setSaslAuthData() and memcache_set_sasl_auth_data()
|
@remicollet thank you so much for pointing to this. I've just added the tests as well as declared username and password fields explicitly. |
| @@ -1546,15 +1573,15 @@ static void php_mmc_set_failure_callback(mmc_pool_t *pool, zval *mmc_object, zva | |||
| callback_tmp = *callback; | |||
| zval_copy_ctor(&callback_tmp); | |||
|
|
|||
| add_property_zval(mmc_object, "_failureCallback", &callback_tmp); | |||
There was a problem hiding this comment.
add_property_zval frees the old value of _failureCallback, but the old code might leak the old value (ZVAL_COPY overwrites without checking the original value). It should be safe to keep the original code (not a performance sensitive part)?
E.g. add tests of calling https://www.php.net/manual/en/memcache.setserverparams.php more than once with a non-null failure callback and run in a debug php build
|
|
||
| zval_ptr_dtor(&callback_tmp); | ||
|
|
||
| pool->failure_callback_param = *mmc_object; | ||
| Z_ADDREF_P(mmc_object); | ||
| } | ||
| else { | ||
| add_property_null(mmc_object, "_failureCallback"); | ||
| ZVAL_NULL(Z_MEMCACHE__FAILURECALLBACK_P(mmc_object)); |
| @@ -1333,7 +1360,7 @@ static void php_mmc_connect(INTERNAL_FUNCTION_PARAMETERS, zend_bool persistent) | |||
| list_res = zend_register_resource(pool, le_memcache_pool); | |||
| mmc_object = return_value; | |||
| object_init_ex(mmc_object, memcache_ce); | |||
| add_property_resource(mmc_object, "connection", list_res); | |||
| ZVAL_RES(Z_MEMCACHE_CONNECTION_P(mmc_object), list_res); | |||
There was a problem hiding this comment.
The original code would free the original property value if something set it before the call to connect (e.g. user manually calling <?php $memcache->connection = new stdClass(); $memcache->connect();)
| pool = mmc_pool_new(); | ||
| pool->failure_callback = (mmc_failure_callback) &php_mmc_failure_callback; | ||
| list_res = zend_register_resource(pool, le_memcache_pool); | ||
| add_property_resource(mmc_object, "connection", list_res); | ||
| ZVAL_RES(Z_MEMCACHE_CONNECTION_P(mmc_object), list_res); |
There was a problem hiding this comment.
original code would free the old value of $memcache->connection, this replacement just overwrites without checking the old value. (e.g. $memcache->connection = new stdClass(); $memcache->addServer(...); would no longer free the stdClass)
It isn't performance sensitive for something that'd be called once total per Memcache instance
|
The rest of the PR looks correct at a glance |
|
Is this still required or #104 is enough? |
|
#104 is enough to make the extension compatible with PHP 8.2. |
Most of issues with PHP 8.2 are caused by dynamic properties deprecations
So in order to resolve the issues I explicitly declared 2 properties in Memcache class. In the future it is possible to add type hint to connection property but it would break BC. (there is even a test which check if connection is possible to set to true)
Also there were some minor incompatibilities in tests which were fixed.
I checked on PHP 8.2.0RC1.
Before the patch:
With this patch: