streams: Fix php_stream_from_{res|zval}_no_verify() APIs actually throwing TypeErrors #18079
Conversation
Girgias
force-pushed
the
stream-no-verify-fix
branch
from
4c7044e to
f645513
Compare
Girgias
force-pushed
the
stream-no-verify-fix
branch
from
f645513 to
e992ebb
Compare
| @@ -771,11 +771,17 @@ static xmlParserInputPtr php_libxml_external_entity_loader(const char *URL, | |||
| } else if (Z_TYPE(retval) == IS_RESOURCE) { | |||
| php_stream *stream; | |||
| php_stream_from_zval_no_verify(stream, &retval); | |||
| if (stream == NULL) { | |||
| if (UNEXPECTED(stream == NULL)) { | |||
There was a problem hiding this comment.
This code fragment just looks like a completely different fix (for stable branches as well)?
| @@ -282,8 +282,8 @@ END_EXTERN_C() | |||
| return; \ | |||
| } \ | |||
| } while (0) | |||
| #define php_stream_from_res_no_verify(xstr, pzval) (xstr) = (php_stream*)zend_fetch_resource2((res), "stream", php_file_le_stream(), php_file_le_pstream()) | |||
| #define php_stream_from_zval_no_verify(xstr, pzval) (xstr) = (php_stream*)zend_fetch_resource2_ex((pzval), "stream", php_file_le_stream(), php_file_le_pstream()) | |||
| #define php_stream_from_res_no_verify(xstr, pzval) (xstr) = (php_stream*)zend_fetch_resource2((res), NULL, php_file_le_stream(), php_file_le_pstream()) | |||
There was a problem hiding this comment.
I don't understand the motivation, the no_verify just means that the macro won't return, so it's just an unclear name.
Also this behaviour change does not seem worth it for extensions?
There was a problem hiding this comment.
The motivation is to have an API that extension can rely on to get a php_stream (mainly from a zval) to ease the migration from resource to objects. And we have a bunch of case which use zend_fetch_resource2 with NULL so they can handle the errors themselves.
I guess I'll just add a new macro/inline function
There was a problem hiding this comment.
I would prefer not doing those incremental changes. Once there is such migration, users will need to handle it anyway.
If we pass a string as the
resource_type_nameparameter instead ofNULLthen a TypeError is emitted.