Migrate ext/dba resources to objects #14239
Conversation
|
We'll be in trouble with the |
There was a problem hiding this comment.
Had a cursory look, as I'm not on my main desktop so don't have all the DBA drivers install, can test it early next week.
Some questions:
- Wasn't there a request for it to implement cast to int?
- Is it really necessary to keep the persistent resource?
Maybe @nielsdos also wants to have a look as they reviewed the ODBC PR.
| $descriptors = [["pty"], ["pty"], ["pty"], ["pipe", "w"]]; | ||
| $pipes = []; | ||
| return proc_open('echo "foo";', $descriptors, $pipes); |
There was a problem hiding this comment.
I don't know if it makes sense to create a resource in ext/zend_test for those sorts of purposes?
There was a problem hiding this comment.
I agree with Gina, I'd use zend_test for this unless that would be too cumbersome.
There was a problem hiding this comment.
Yes, a dedicated test-only resource is our last resort as soon as we run out of regular non-stream resources. Until 9.0, we are fine with proc_open(), so I'd postpone this hassle until then, if you are ok with it.
| char *resource_key; | ||
| size_t resource_key_len = spprintf(&resource_key, 0, | ||
| "dba_%d_%s_%s_%s", persistent, ZSTR_VAL(path), ZSTR_VAL(mode), handler_str ? ZSTR_VAL(handler_str) : "" | ||
| ); |
There was a problem hiding this comment.
Why not strpprintf()/zend_strpprintf() to get a zend_string directly?
There was a problem hiding this comment.
I copy-pasted this piece of code from ext/odbc :) But I didn't really know about these APIs to be honest, so that's the main reason.
There was a problem hiding this comment.
BTW we sometimes need to create persistent strings, which this API doesn't support as far as I saw, so at the end of the day, we would have to create a new zend_string in any case when dealing with persistent resources.
ext/dba/dba.c
Outdated
| zend_hash_add_new(&DBA_G(connections), connection->hash, return_value); | ||
| FREE_RESOURCE_KEY(); | ||
| return; |
There was a problem hiding this comment.
I'm confused here, can't we get rid of the persistent resource by storing the pointer for the DBH in the zval instead of the constructed object?
|
I can't have a proper look today, maybe tomorrow. |
Ah, that's still more than sufficient :) I appreciate your help! |
Oh yes, thanks for reminding me. I'll create a followup for odbc as well.
Hmm, I don't know what the proper way would be to migrate persistent resources would be... For now, I went with the usual implementation. But I'm fine to do followups if we come up with a universal solution. |
ext/dba/dba.c
Outdated
| @@ -469,13 +521,12 @@ static zend_always_inline zend_string *php_dba_zend_string_dup_safe(zend_string | |||
| } | |||
|
|
|||
|
|
|||
| #define FREE_PERSISTENT_RESOURCE_KEY() if (persistent_resource_key) {zend_string_release_ex(persistent_resource_key, false);} | |||
| #define FREE_RESOURCE_KEY() efree(resource_key); | |||
ext/dba/dba.c
Outdated
| if ((le = zend_hash_index_find_ptr(&EG(regular_list), i)) == NULL) { | ||
| continue; | ||
| zval *zv; | ||
| ZEND_HASH_FOREACH_VAL(&DBA_G(connections), zv) { |
There was a problem hiding this comment.
Because the keys in the connections HashTable are all strings, the underlying structure will be an actual hashmap.
So you can use ZEND_HASH_MAP_FOREACH_VAL for better looping performance and better code compaction.
The same holds for other loops over the connections able too.
ext/dba/dba.c
Outdated
| } | ||
| */ | ||
| static zend_class_entry *dba_connection_ce; | ||
| static zend_object_handlers dba_connection_object_handlers; | ||
| /* }}} */ |
There was a problem hiding this comment.
Dangling }}} folding marker that may be removed.
ext/dba/dba.c
Outdated
| } | ||
| */ | ||
| static zend_class_entry *dba_connection_ce; | ||
| static zend_object_handlers dba_connection_object_handlers; | ||
| /* }}} */ | ||
|
|
||
| /* {{{ dba_close */ |
There was a problem hiding this comment.
Dangling folding marker that may be removed.
Related to https://wiki.php.net/rfc/resource_to_object_conversion and php/php-tasks#6