Skip to content
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

Add dedicated StreamBucket class #13111

Merged
merged 4 commits into from Apr 11, 2024
Merged

Conversation

kocsismate
Copy link
Member

@kocsismate kocsismate commented Jan 9, 2024

Copy link
Member

@bukka bukka left a 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

ext/standard/user_filters.c Show resolved Hide resolved
ext/standard/user_filters.c Show resolved Hide resolved
ext/standard/user_filters.c Show resolved Hide resolved
ext/standard/basic_functions.stub.php Show resolved Hide resolved
ext/standard/user_filters.stub.php Outdated Show resolved Hide resolved
ext/standard/user_filters.stub.php Outdated Show resolved Hide resolved
Copy link
Member

@bukka bukka left a 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.

ext/standard/user_filters.c Show resolved Hide resolved
ext/standard/user_filters.c Show resolved Hide resolved
ext/standard/basic_functions.stub.php Show resolved Hide resolved
@@ -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);
Copy link
Contributor

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.

Copy link
Member Author

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.

ext/standard/user_filters.c Show resolved Hide resolved
ext/standard/user_filters.c Outdated Show resolved Hide resolved
Copy link
Member

@bukka bukka left a 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.

@kocsismate kocsismate merged commit be2f454 into php:master Apr 11, 2024
10 checks passed
@kocsismate kocsismate deleted the stream-bucket-class branch April 11, 2024 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants