-
Notifications
You must be signed in to change notification settings - Fork 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
bukka
left a comment
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
bukka
left a comment
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
| <?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
bukka
left a comment
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