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é
Hi,
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()andstream_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()andstream_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
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é
Hi,
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()andstream_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
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é
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
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