Hi, On Wed, Jan 17, 2024 at 9:14 PM Máté Kocsis <kocsismat...@gmail.com> wrote:
> 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). >> > > I agree with using a two step migration approach, and that's why I only > changed one part for now: > the containing object, leaving its resource property intact. However, as > far as I can tell, > there's no need to create a separate StreamBucketHandle object when > migrating the stream bucket resource to an > object, but we could inline it directly into the containing StreamBucket > object. I'm saying this because the > stream bucket resource property is only used by two functions: > stream_bucket_append() and stream_bucket_prepend(), > and I cannot imagine any other use-case where having a > StreamBucket::$bucket property would be useful (but correct > me if I'm wrong, I'm not the most experienced with stream buckets). > > I read again the PR and not sure why I thought that it's supposed to replace the bucket resource as it's just about replacing stdClass with StreamBucket. The thing that probably confused me is that the actual stream bucket is just a property $bucket in that object currently (resource handle) and not the wrapping object that you are changing. I kind of imagined to do it other way round but maybe this makes more sense. > 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. >> > > For me, it seems like a subtle edge case which could be addressed by > explicitly mentioning the change in the UPGRADING file. > But I got your point, and I'm ok to submit an RFC. > > The only issue that I see that if you migrate the resource to object you effectively drop that property which might be a BC break but based on the recent RFC results it will happen in PHP 9.0 so it's not such a big issue. I think this might be actually an opportunity to deprecate the usage of that property. So if this targeted 8.4, then it would allow us to keep the $bucket property there but also deprecate its accessing so users can migrate the unlikely use of this property. Other option would be to keep the property a an object self-reference but that would be pointless and not nice. I think the RFC should be done for this though. Regards Jakub
