Updates for PHP 7.3 #882
Updates for PHP 7.3 #882
Conversation
tests/volatile-arrays.phpt
Outdated
| @@ -54,7 +56,7 @@ object(Threaded)#%d (%d) { | |||
| } | |||
| } | |||
| } | |||
| [0]=> | |||
| ["0"]=> | |||
There was a problem hiding this comment.
@sirsnyder are you sure we want to ignore this instead of correcting it? It is a BC break after all.
There was a problem hiding this comment.
@dktapps nope, my fault. I assumed this would be the new and intended behavior of php arrays. But it's not, will have a look at it.
|
@sirsnyder Looks good to me 👍 |
|
This isn't synced with the latest changes, I haven't had time to update it yet. |
|
@dktapps @sirsnyder Any updates on this? Release of 7.3 is just around the door. |
|
@CharlotteDunois I haven't touched this since the 7.3 alphas. I'll take another look at it soon when I find time. |
|
I'm busy with a huge feature for pthreads in the last few weeks. Would be cool if you can take care of it @dktapps |
| @@ -595,7 +615,7 @@ static inline void pthreads_prepare_constants(pthreads_object_t* thread) { | |||
| case IS_DOUBLE: Z_DVAL(constant.value)=Z_DVAL(zconstant->value); break; | |||
| case IS_STRING: { | |||
| #if PHP_VERSION_ID >= 70300 | |||
| Z_STR(constant.value)=Z_STR(zconstant->value); | |||
| Z_STR(constant.value)= zend_string_new(Z_STR(zconstant->value)); | |||
| #else | |||
| ZVAL_NEW_STR(&constant.value, zend_string_new(Z_STR(zconstant->value))); | |||
| #endif | |||
There was a problem hiding this comment.
This block probably does not need the if/else.
| @@ -1030,15 +1039,30 @@ void pthreads_store_reset(zval *object, HashPosition *position) { | |||
|
|
|||
| if (pthreads_monitor_lock(threaded->monitor)) { | |||
| zend_hash_internal_pointer_reset_ex(threaded->store.props, position); | |||
| if (zend_hash_has_more_elements_ex(threaded->store.props, position) == FAILURE) { //empty | |||
| *position = HT_INVALID_IDX; | |||
There was a problem hiding this comment.
This is duct tape and should probably be fixed better later.
There was a problem hiding this comment.
Yes, it's not a beauty but it is working for now. And who knows when they will change this in php again.
ghost
mentioned this pull request
This bug was caused by changes made in php/php-src@d7f2dc4 There are probably better solutions for this, but this was the simplest and least invasive fix I could come up with.
This issue is described in #1. I think move_forward from a non existing index to an existing one is probably undefined behaviour anyway, so this PR prefers using reset, which is also backwards compatible with 7.2.
use interned strings for globals fixed one incorrect use of persistent string work around possible bug in php-src in zend_hash_str_update_ptr() - keys inserted like this inherit persistence from their parent htable, which is not expected for the global function_table
Previously this would yield persistent strings from the props table. Inserting these strings into standard arrays would then cause issues later on.
|
I've run some tests and this seems to be ready to r&r. I'm sure more bugs will roll in later on. @sirsnyder I think this is good to merge. |
|
@dktapps that's nice to hear. Two days back I've played with debug builds and two tests have mem leaks. The one I've investigated is in |
This is mostly just fixing issues inserting persistent strings into non-persistent hashtables. In some cases I have replaced them with interned strings, others just needed copying appropriately.
There are also fixes for iteration.
There is a BC break in the behaviour of Volatile object keys which I believe is caused by php/php-src#3351. I haven't taken the time to try and fix this yet.