Re: [PHP-DEV] Dedicated StreamBucket class
On Fri, 19 Jan 2024 at 16:04, Sara Golemon wrote: > > No disrespect to all the folks (including myself) who had a part in file > I/O as it exists today, but it *IS* a hot mess. I've sketched out > redesigns with folks over the years, but I have to be honest that I don't > have the spoons to invest in Stream 2.0, nor do most of the > fellow-grayhairs I've chatted with about it. > FWIW - I had forgotten about Streams 2 until now, and this may or may not be relevant - me and Sara had started fleshing out what a redesigned Stream Objects API might look like: https://github.com/phplang/abandoned-streams2 It had completely slipped my memory :D
Re: [PHP-DEV] Dedicated StreamBucket class
On Sat, Jan 13, 2024 at 1:29 AM Máté Kocsis wrote: > 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. > I don't want to stand in the way of you improving this part of Streams, and I'm really not commenting on your PR at all. BUT I would like to formally ask/give-permission to just redesign file I/O in PHP altogether. This would be a nice feature drop for 9.0, and we could deprecate the old stuff in 10, remove it in 12? No disrespect to all the folks (including myself) who had a part in file I/O as it exists today, but it *IS* a hot mess. I've sketched out redesigns with folks over the years, but I have to be honest that I don't have the spoons to invest in Stream 2.0, nor do most of the fellow-grayhairs I've chatted with about it. So please, if you at all feel inclined, burn this psuedo-posix I/O hell we have to the ground and replace it with the gleaming phoenix that PHP deserves. -Sara
Re: [PHP-DEV] Dedicated StreamBucket class
Hi Jakub, > 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. > Totally agreed! Actually I have already written the RFC text with the same proposal for deprecating the property. :) Will share it in a new thread to start the discussion period. Thank you for your feedback, Máté >
Re: [PHP-DEV] Dedicated StreamBucket class
Hi, On Wed, Jan 17, 2024 at 9:14 PM Máté Kocsis 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
Re: [PHP-DEV] Dedicated StreamBucket class
Hi Jakub, 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 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. Regards, Máté
Re: [PHP-DEV] Dedicated StreamBucket class
Hi, On Sat, Jan 13, 2024 at 9:29 AM Máté Kocsis 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
[PHP-DEV] Dedicated StreamBucket class
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. 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. 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? Regards, Máté
