Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:122187
MIME-Version: 1.0
References: <CAH5C8xXPNz1e2EARB5HpuiWpuRh=qRj8r1ZgpCH-=UFRJ_9O=Q@mail.gmail.com>
 <CAEKnhAEHuA-r+fwNVK7Mv8P43N4F8z+4QtXz1DnQawrtQyCw+w@mail.gmail.com> <CAH5C8xUR2Sky3Xf8p+NTMj_+RgQfkrMn0YUDd_=4K=-8qp8KEA@mail.gmail.com>
In-Reply-To: <CAH5C8xUR2Sky3Xf8p+NTMj_+RgQfkrMn0YUDd_=4K=-8qp8KEA@mail.gmail.com>
Date: Fri, 19 Jan 2024 11:10:19 +0000
Message-ID: <CAEKnhAFQpC-WiCWZURimbQqhibmOPMNb1sRHqF48BQqcQxfYJQ@mail.gmail.com>
To: =?UTF-8?B?TcOhdMOpIEtvY3Npcw==?= <kocsismate90@gmail.com>
Cc: PHP Internals List <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000003b22e7060f4a87c5"
Subject: Re: [PHP-DEV] Dedicated StreamBucket class
From: bukka@php.net (Jakub Zelenka)

--0000000000003b22e7060f4a87c5
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Wed, Jan 17, 2024 at 9:14=E2=80=AFPM M=C3=A1t=C3=A9 Kocsis <kocsismate90=
@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 t=
wo
>> 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 curren=
t
>> use of bucket resource. The actual StreamBucket could be introduced late=
r
>> (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 u=
sed
>> for copying but seems quite unlikely. Still I would see this as a BC bre=
ak
>> 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

--0000000000003b22e7060f4a87c5--