Make php_fgetcsv() return a HashTale instead of in-out zval param #8936
Conversation
Also refactor what happens on an empty line to return NULL instead of setting the array to [NULL] which makes no design sense at all. However, as this is the current behaviour create a BC Shim macro to recreate this weird HashTable in the function which currently use this API
There was a problem hiding this comment.
I think in case of an empty line [""] should be returned (note that a line with a single delimiter returns ["", ""]). Anyhow, I agree that we can't change that for BC reasons, so I'm not completely convinced that we should change the API, although I see some benefit for extensions using the it.
ext/standard/file.h
Outdated
| #define BC_EMPTY_CSV_LINE_ARRAY(values) { (values) = zend_new_array(1); zval _______z_tmp; \ | ||
| ZVAL_NULL(&_______z_tmp); zend_hash_next_index_insert((values), &_______z_tmp); } |
There was a problem hiding this comment.
It seems to me an inline function would be preferable.
The main reason why I want to change the API is for SplFileObject, as currently it's not skipping on empty lines if the flag is provided, and I don't know how to check for this absurd case in a good way. |
There was a problem hiding this comment.
Other than the _______z_tmp which could be solved with an inline function this LGTM. Although to be honest I'm not sure there's an inherent benefit to returning HashTable and making usage more complicated, unless the function will be used in places that doesn't use zvals directly in the future.
Right, currently SplFileObject stores a zval (I think) only for this use case which could be dropped to use a HashTable and this is mainly a "private" API used within php-src |
|
I think there should be a note in UPGRADING.INTERNALS. |
Also refactor what happens on an empty line to return NULL instead of setting the array to [NULL] which makes no design sense at all.
However, as this is the current behaviour create a BC Shim macro to recreate this weird HashTable in the function which currently use this API