Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:122971
Precedence: bulk
MIME-Version: 1.0
References: <CAF-fPAk0JxToi=dEupJHOEhsb3QiA-M1x_9znAFXfDSbCxvNzQ@mail.gmail.com>
 <CAPyj-LCLc4XF=yndX8ecT1xnTP=yzziUm4ttu86uKMfdF=8W5g@mail.gmail.com>
In-Reply-To: <CAPyj-LCLc4XF=yndX8ecT1xnTP=yzziUm4ttu86uKMfdF=8W5g@mail.gmail.com>
Date: Fri, 5 Apr 2024 16:20:38 +0200
Message-ID: <CAF-fPA=mEoVTkPRJhZ7cZjtuaeraNbBBbAQfDTwy_0Aggk7g0Q@mail.gmail.com>
Subject: Re: [PHP-DEV] Proposal: retrieve line, filename and if user defined
 for ReflectionAttribute
To: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="00000000000096392606155a29dc"
From: jwurtz@jolicode.com (Joel Wurtz)

--00000000000096392606155a29dc
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

> Would it make sense to not only add this for ReflectionAttribute, but
also Function and/or others?

There may be case where this makes sense but not necessarily in the use
case that i explain, and don't want to add more to this proposal, it's also
missing in ReflectionParameter, ReflectionProperty and certainly many
others if we go on this path then there is a lot of changes to apply and
will induce more friction for this proposal, i prefer to add small parts so
we don't deviate from the original use case.

> Do you have an example of how you expect this to be used?  I'm having a
hard time understanding how I'd leverage this.  (I maintain an attribute
enhancement library, Crell/AttributeUtils, so I've hit a lot of edge cases
in attribute design.)

Sure, let's say we have the following class

#[CustomAttribute(foo: 'foo')
#[CustomAttribute(foo: 'foo')
class Entity {}

And that later i have something that read those attributes, but i want to
throw an Exception when multiple attribute defined the same value for the
foo property (as it should be unique in my use case), i would not be able
to do that in the constructor since i don't have the context of others
attributes in this case, so i want to throw a custom exception explaining
it's caused by the attribute "CustomAttribute" in the file "Entity.php" at
line 4, as there is the same value in the file "Entity.php" at line 3

If I throw an exception without this message the error will pinpoint to
where I threw the exception and it may be confusing for the end user, by
having this he will know where to look and what to modify to make its code
correct.

> I would propose negating and renaming isUserDefined() to isInternal()

Both methods exists depending the context (there is a isUserDefined() in
the ReflectionClass class), but i'm fine with negating this if it's more
consistent

> As hinted in the PR, I believe the implementation is wrong,

I think the implementation is correct, i answered in the PR, i will add
more tests case that explain what is wanted here

> I'm also wondering whether it may be more useful to expose the attribute
class as getClass()

Do we not already have this with the getName() method ? or it can be wrong
if it's an alias by example ?

Jo=C3=ABl

--00000000000096392606155a29dc
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr"><div><div><div>&gt; Would it make sense t=
o not only add this for ReflectionAttribute, but also Function and/or other=
s?<br><br></div>There
 may be case where this makes sense but not necessarily in the use case=20
that i explain, and don&#39;t want to add more to this proposal, it&#39;s a=
lso=20
missing in ReflectionParameter, ReflectionProperty and certainly many=20
others if we go on this path then there is a lot of changes to apply and
 will induce more friction for this proposal, i prefer to add small=20
parts so we don&#39;t deviate from the original use case.<br><br>&gt; Do yo=
u have an example of how you expect this to be used?=C2=A0 I&#39;m having a=
=20
hard time understanding how I&#39;d leverage this.=C2=A0 (I maintain an att=
ribute
 enhancement library, Crell/AttributeUtils, so I&#39;ve hit a lot of edge=
=20
cases in attribute design.)<br><br></div>Sure, let&#39;s say we have the fo=
llowing class<div><br></div><div>#[CustomAttribute(foo: &#39;foo&#39;)</div=
><div>#[CustomAttribute(foo: &#39;foo&#39;)<br></div>class Entity {}<font c=
olor=3D"#888888"><br></font><div><br></div><div>And
 that later i have something that read those attributes, but i want to=20
throw an Exception when multiple attribute defined the same value for=20
the foo property (as it should be unique in my use case), i would not be ab=
le to do=20
that in the constructor since i don&#39;t have the context of others=20
attributes in this case, so i want to throw a custom exception explaining i=
t&#39;s caused by the attribute &quot;CustomAttribute&quot; in
 the file &quot;Entity.php&quot; at line 4, as there is the same value in t=
he=20
file &quot;Entity.php&quot; at line 3<br></div><br></div><div>If I throw an=
=20
exception without this message the error will pinpoint to where I threw=20
the exception and it may be confusing for the end user, by having this he w=
ill know where to look and what to modify to make its code correct.<br><br>=
&gt; I would propose negating and renaming isUserDefined() to isInternal()<=
br><br></div><div>Both methods exists depending the context (there is a isU=
serDefined() in the ReflectionClass class), but i&#39;m fine with negating =
this if it&#39;s more consistent<br><br>&gt; As hinted in the PR, I
believe the implementation is wrong,<br><br></div><div>I think the implemen=
tation is correct, i answered in the PR, i will add more tests case that ex=
plain what is wanted here<br><br>&gt; I&#39;m also wondering whether it may=
 be more useful to expose the attribute class as getClass()<br><br></div><d=
iv>Do we not already have this with the getName() method ? or it can be wro=
ng if it&#39;s an alias by example ?<br><br></div><div>Jo=C3=ABl<br></div><=
/div></div>

--00000000000096392606155a29dc--