Hi, On Sat, Jan 13, 2024 at 9:29 AM Máté Kocsis <kocsismat...@gmail.com> wrote:
> Hi Everyone, > > Recently, I realized that the stream_bucket_new() and > stream_bucket_make_writeable() functions > create stdClass instances by dynamically adding a "bucket", a "data" and a > "datalen" property to it. > > A few days ago, I submitted a PR which makes the above mentioned functions > return a dedicated StreamBucket class which has these parameters properly > declared (https://github.com/php/php-src/pull/13111/). Furthermore, > stream_bucket_prepend() and stream_bucket_append() would accept objects of > this class as their second parameter from now on. Before, they accepted any > kind of objects as long as they had a "bucket" property containing a valid > stream. > > I just submitted feedback to the PR but will also mention it here as it's probably more an API thing. The problem that I see is that it combines two distinct things and create quite ugly self reference inside the proposed StreamBucket object. I would prefer we split it and introduce StreamBucketHandle opaque object that will completely replace the current use of bucket resource. The actual StreamBucket could be introduced later (ideally in PHP 9 as it's a sort of breaking change - change of class). > As far as I see, my changes are backward compatible as long as people use > the stream bucket API properly (i.e. create stream buckets via > stream_bucket_new() and stream_bucket_make_writeable()). If they manually > construct such objects (i.e. $bucket = new stdClass(); $bucket->bucket = > ....) then obviously, they would start to face type errors. > I think it's more typing issue if someone passes this object for further processing and type hint stdClass. Possibly the pattern above could be used for copying but seems quite unlikely. Still I would see this as a BC break and it is not really related to resource to object migration. > > So my question is whether anyone has any use-case/preexisting code which > falls into the second case? If no one knows about the invalid usage > pattern, my next question would be whether I have to create an RFC for this > change? > > If you want to change the API and replace stdClass with StreamBucket, then I think it should have a separate RFC. If you just introduce StreamBucketHandle as suggested, then I think it's already covered by other RFC as it's just a simple resource to object migration. Regards Jakub
