-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Add dedicated StreamBucket class #13111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not right way. You are trying to use StreamBucket for two distinct things. I think the primary work here should be to get rid of resource bucket object. This is visible to users only through the bucket property. I would imaging we should rather call it StreamBucketHandle and it should be opaque. Another thing is replacing the actual object returned by changed function which is currently just stdClass. It would make sense to eventually change it to typed class that could be called StreamBucket but that's really something different and those two things should not be combined together IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I somehow thought that it's supposed to replace the stream bucket resource which is not the case here.
3b570e2
to
20ff9ea
Compare
20ff9ea
to
1db667c
Compare
@@ -4,6 +4,7 @@ Bug #39551 (Segfault with stream_bucket_new in user filter) | |||
<?php | |||
|
|||
$bucket = stream_bucket_new(fopen('php://temp', 'w+'), ''); | |||
var_dump($bucket); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you're really only interested in the class, perhaps use:
echo get_class($bucket), "\n";
Then you don't have to update the test later when bucket
becomes an object, or datalen
becomes dataLength
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good idea if there were other tests which ensured the structure of StreamBucket
. But I'd prefer to keep this test as-is in this case, since that's the only place where the properties are displayed.
1db667c
to
26ae64d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me.
RFC: https://wiki.php.net/rfc/dedicated_stream_bucket