Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:125043
Feedback-ID: ifab94697:Fastmail
Precedence: bulk
MIME-Version: 1.0
Date: Sun, 18 Aug 2024 23:50:43 +0200
To: "Stephen Reay" <php-lists@koalephant.com>
Cc: "PHP internals" <internals@lists.php.net>
Message-ID: <6cc30222-5636-48a6-be67-622954d42e86@app.fastmail.com>
In-Reply-To: <44486322-F313-405C-91D0-EA13FCAD9C65@koalephant.com>
References: <a897e9e9-dcc7-47f9-86db-c3c15294046a@app.fastmail.com>
 <94259551-80EE-41F4-9CF9-679B79B5EA49@koalephant.com>
 <c7782917-55ce-4d5b-bfad-2048d9a0af2d@app.fastmail.com>
 <44486322-F313-405C-91D0-EA13FCAD9C65@koalephant.com>
Subject: Re: [PHP-DEV] function autoloading v4 RFC
Content-Type: multipart/alternative;
 boundary=19fcce9433be45408496ba85f6458830
From: rob@bottled.codes ("Rob Landers")

--19fcce9433be45408496ba85f6458830
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

On Sun, Aug 18, 2024, at 11:36, Stephen Reay wrote:
>=20
> Hi Rob,
>=20
>> On 18 Aug 2024, at 04:33, Rob Landers <rob@bottled.codes> wrote:
>>=20
>> I wouldn't consider it a BC break, no. But (ironically?), Symfony cra=
shes with this change.
>=20
>=20
> I wasn't aware of that specific code before but it's exactly the type =
of issue I was talking about earlier.
>=20
>> Ah, good catch. I've updated this and gone through other relevant fun=
ctions.
> The RFC now says "The spl_autoload function will not be updated.", but=
 that will *also* break if it isn't updated to at least *account for*, e=
ven if it doesn't *use* the second argument given.
>=20
> However I'm also curious why you would *specifically* make it *not* su=
pport function loading?
> The current implementation should work unmodified, once the signature =
is changed to accept an int as the second parameter (and move the curren=
t 2nd parameter to 3rd),  There is nothing "class specific" in the exist=
ing implementation except for a couple of variable names.
>=20
>=20
> I would imagine you also need to update spl_autoload_unregister[1] - i=
t also needs to be able to identify the type of autoloader it's operatin=
g on (because the same autoloader might be defined for both types).
>=20
> And lastly I think you'd need to adapt spl_autoload_functions[2] too -=
 perhaps the same as the others, introduce a parameter `int $type =3D SP=
L_AUTOLOAD_CLASS`, so existing code works as-is, otherwise it'd be impos=
sible to know how a given autoloader was registered.
>=20
>=20
> 1: https://www.php.net/manual/en/function.spl-autoload-unregister.php
> 2: https://www.php.net/manual/en/function.spl-autoload-functions.php
>=20
>=20
> Cheers
>=20
> Stephen=20

Hello again internals,

The implementation has now reached a stable point and things discovered =
during the implementation have been reflected in the RFC text itself.

I did my best to assess the impact to Opcache, but I suspect someone muc=
h more familiar with how opcache works will need to take a look. From my=
 understanding of the changes needed in zend_execute, it looks like ther=
e are some changes needed to the JIT helpers, but there may be more chan=
ges required that aren't so obvious. I've added a helper that can be use=
d as a drop-in replacement for looking up functions from the function ta=
ble directly, which should help.

Lastly, I do plan to open a docs PR in the coming week that will probabl=
y trigger some smaller updates to the RFC; mostly to smooth out the word=
ing and make it more friendly for non-technical people, but the technica=
l aspects shouldn't change (barring the discussion here, of course).

> The RFC now says "The spl_autoload function will not be updated.", but=
 that will *also* break if it isn't updated to at least *account for*, e=
ven if it doesn't *use* the second argument given.

I've updated the text to reflect exactly that, it did require updating. =
;)

> However I'm also curious why you would *specifically* make it *not* su=
pport function loading?
> The current implementation should work unmodified, once the signature =
is changed to accept an int as the second parameter (and move the curren=
t 2nd parameter to 3rd),  There is nothing "class specific" in the exist=
ing implementation except for a couple of variable names.

I mostly decided not to support it to avoid the inevitable bikeshedding =
of how a "default function autoloader" would work. I really want to push=
 that to a separate RFC, unless there was a general consensus of an impl=
ementation. If we are fine reusing the existing default class autoloadin=
g, then I am fine with that.

> And lastly I think you'd need to adapt spl_autoload_functions[2] too -=
 perhaps the same as the others, introduce a parameter `int $type =3D SP=
L_AUTOLOAD_CLASS`, so existing code works as-is, otherwise it'd be impos=
sible to know how a given autoloader was registered.

I've also added those functions as well.

=E2=80=94 Rob
--19fcce9433be45408496ba85f6458830
Content-Type: text/html; charset=utf-8
Content-Transfer-Encoding: quoted-printable

<!DOCTYPE html><html><head><title></title><style type=3D"text/css">p.Mso=
Normal,p.MsoNoSpacing{margin:0}</style></head><body><div>On Sun, Aug 18,=
 2024, at 11:36, Stephen Reay wrote:<br></div><blockquote type=3D"cite" =
id=3D"qt" style=3D"overflow-wrap:break-word;"><div><br></div><div><div s=
tyle=3D"color:rgb(0, 0, 0);">Hi Rob,<br></div><div><br></div><blockquote=
 type=3D"cite"><div>On 18 Aug 2024, at 04:33, Rob Landers &lt;rob@bottle=
d.codes&gt; wrote:<br></div><div><br></div><div><span style=3D"font-styl=
e:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;=
text-align:start;text-indent:0px;text-transform:none;text-wrap:wrap;word=
-spacing:0px;-webkit-text-stroke-width:0px;text-decoration-line:none;tex=
t-decoration-thickness:initial;text-decoration-style:initial;text-decora=
tion-color:initial;float:none;display:inline !important;"><span class=3D=
"font" style=3D"font-family:Helvetica;"><span class=3D"size" style=3D"fo=
nt-size:12px;">I wouldn't consider it a BC break, no. But (ironically?),=
 Symfony crashes with this change.</span></span></span><br></div></block=
quote></div><div><br></div><div><br></div><div>I wasn't aware of that sp=
ecific code before but it's exactly the type of issue I was talking abou=
t earlier.<br></div><div><br></div><div><div><blockquote type=3D"cite">A=
h, good catch. I've updated this and gone through other relevant functio=
ns.<br></blockquote></div></div><div>The RFC now says "The spl_autoload =
function will not be updated.", but that will *also* break if it isn't u=
pdated to at least *account for*, even if it doesn't *use* the second ar=
gument given.<br></div><div><br></div><div>However I'm also curious why =
you would *specifically* make it *not* support function loading?<br></di=
v><div>The current implementation should work unmodified, once the signa=
ture is changed to accept an int as the second parameter (and move the c=
urrent 2nd parameter to 3rd), &nbsp;There is nothing "class specific" in=
 the existing implementation except for a couple of variable names.<br><=
/div><div><br></div><div><br></div><div>I would imagine you also need to=
 update spl_autoload_unregister[1] - it also needs to be able to identif=
y the type of autoloader it's operating on (because the same autoloader =
might be defined for both types).<br></div><div><br></div><div>And lastl=
y I think you'd need to adapt spl_autoload_functions[2] too - perhaps th=
e same as the others, introduce a parameter `int $type =3D SPL_AUTOLOAD_=
CLASS`, so existing code works as-is, otherwise it'd be impossible to kn=
ow how a given autoloader was registered.<br></div><div><br></div><div><=
br></div><div>1:&nbsp;<a href=3D"https://www.php.net/manual/en/function.=
spl-autoload-unregister.php">https://www.php.net/manual/en/function.spl-=
autoload-unregister.php</a><br></div><div>2:&nbsp;<a href=3D"https://www=
.php.net/manual/en/function.spl-autoload-functions.php">https://www.php.=
net/manual/en/function.spl-autoload-functions.php</a><br></div><div><br>=
</div><div><br></div><div>Cheers<br></div><div><br></div><div>Stephen&nb=
sp;<br></div></blockquote><div><br></div><div>Hello again internals,<br>=
</div><div><br></div><div>The implementation has now reached a stable po=
int and things discovered during the implementation have been reflected =
in the RFC text itself.<br></div><div><br></div><div>I did my best to as=
sess the impact to Opcache, but I suspect someone much more familiar wit=
h how opcache works will need to take a look. From my understanding of t=
he changes needed in zend_execute, it looks like there are some changes =
needed to the JIT helpers, but there may be more changes required that a=
ren't so obvious. I've added a helper that can be used as a drop-in repl=
acement for looking up functions from the function table directly, which=
 should help.<br></div><div><br></div><div>Lastly, I do plan to open a d=
ocs PR in the coming week that will probably trigger some smaller update=
s to the RFC; mostly to smooth out the wording and make it more friendly=
 for non-technical people, but the technical aspects shouldn't change (b=
arring the discussion here, of course).<br></div><div><br></div><blockqu=
ote type=3D"cite" id=3D"qt" style=3D"overflow-wrap:break-word;"><div>The=
 RFC now says "The spl_autoload function will not be updated.", but that=
 will *also* break if it isn't updated to at least *account for*, even i=
f it doesn't *use* the second argument given.<br></div></blockquote><div=
><br></div><div>I've updated the text to reflect exactly that, it did re=
quire updating. ;)<br></div><div><br></div><blockquote type=3D"cite" id=3D=
"qt" style=3D"overflow-wrap:break-word;"><div>However I'm also curious w=
hy you would *specifically* make it *not* support function loading?<br><=
/div><div>The current implementation should work unmodified, once the si=
gnature is changed to accept an int as the second parameter (and move th=
e current 2nd parameter to 3rd), &nbsp;There is nothing "class specific"=
 in the existing implementation except for a couple of variable names.<b=
r></div></blockquote><div><br></div><div>I mostly decided not to support=
 it to avoid the inevitable bikeshedding of how a "default function auto=
loader" would work. I really want to push that to a separate RFC, unless=
 there was a general consensus of an implementation. If we are fine reus=
ing the existing default class autoloading, then I am fine with that.<br=
></div><div><br></div><blockquote type=3D"cite" id=3D"qt" style=3D"overf=
low-wrap:break-word;"><div>And lastly I think you'd need to adapt spl_au=
toload_functions[2] too - perhaps the same as the others, introduce a pa=
rameter `int $type =3D SPL_AUTOLOAD_CLASS`, so existing code works as-is=
, otherwise it'd be impossible to know how a given autoloader was regist=
ered.<br></div></blockquote><div><br></div><div>I've also added those fu=
nctions as well.<br></div><div><br></div><div id=3D"sig121229152">=E2=80=
=94 Rob<br></div></body></html>
--19fcce9433be45408496ba85f6458830--