main: refactor userstream method calling #19312
Conversation
901fbdb to
d926c36
Compare
|
I will need a bit of time to review this properly. Will see if I manage to do it before feature freeze. |
|
Added that to my list of things so should be able to take a look in the next 2 weeks. |
541e885 to
a6fa243
Compare
There was a problem hiding this comment.
I went through and the changes look fine even though it will be a pain if we have some bug fixes (there are no open bugs for user wrapper that could go to stable version at least).
The only issues (except one changed error code) are pretty much just comments that can be just removed as they are pointless anyway.
main/streams/userspace.c
Outdated
|
|
||
| case PHP_STREAM_OPTION_TRUNCATE_API: | ||
| ZVAL_STRINGL(&func_name, USERSTREAM_TRUNCATE, sizeof(USERSTREAM_TRUNCATE)-1); | ||
| return PHP_STREAM_OPTION_RETURN_ERR; |
There was a problem hiding this comment.
Not that it would matter but it's changing NOTIMPL to ERR if I read it correctly.
There was a problem hiding this comment.
It would be better to not do changes into return codes here just in case.
This was initially introduced with php@9f86cdaf7fc4 However, this should also have been done for the opendir call. This omission was found via OSS-Fuzz 51047 [1] and fixed in a more general way in php@d0b3096 by resetting `FG(user_stream_current_filename)` at the end of the request during shutdown. As such this zend_try/zend_catch block is now unnecessary. [1]: https://issues.oss-fuzz.com/issues/42515581
a6fa243 to
13ba537
Compare
|
Not exactly sure why I need RM approval? This is not a user facing change, but anyway I'll ping the 8.5 RMs. @php/release-managers-85 |
|
This is not a bug fix and any feature in beta requires RM approval. |
|
Basically RM is supposed to make sure that any refactoring like this is not going to impact of stability of the release. |
|
@NattyNarwhal Sorry that was misclick... |
There are a few to-dos with questions, as the existing code can be sometimes slightly confusing.