Uh oh!
There was an error while loading. Please reload this page.
Migrate ext/dba resources to objects - #14239
Conversation
We'll be in trouble with the |
Girgias
left a comment
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.
| 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?
ndossche
commented
May 15, 2024
I can't have a proper look today, maybe tomorrow. |
kocsismate
commented
May 15, 2024
Ah, that's still more than sufficient :) I appreciate your help! |
kocsismate
commented
May 15, 2024
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. |
ndossche
left a comment
There was a problem hiding this comment.
I only have nits. Looks good overall.
I tested with GDBM.
| #define FREE_PERSISTENT_RESOURCE_KEY() if (persistent_resource_key) {zend_string_release_ex(persistent_resource_key, false);} | ||
| #define FREE_RESOURCE_KEY() efree(resource_key); |
| 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.
| */ | ||
| 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.
| 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