Everything

php

Home » Project » Newsgroups » php.internals » Proposal: retrieve line, filename and if user defined for ReflectionAttribute »

Proposal: retrieve line, filename and if user defined for ReflectionAttribute

php.internals

Joel Wurtz

2 years ago

Hello everyone,

Like a lot of libraries, we offer the possibility to configure behaviors
with Attributes. However in some cases it's wrongly configured by the user
and this wrong configuration cannot be detected on the attribute
constructor but afterwards.

In this case we may want to pinpoint which attribute (in which file and at
which line) cause this bad configuration. Since there was no method to
retrieve those information in the ReflectionAttribute I proposed a PR
https://github.com/php/php-src/pull/13889 to add those informations.

I do believe this will allow better DX for end user when correctly used,

Regards,

Joël Wurtz

--
Joël Wurtz
06 23 64 03 55

Jolicode, Expertise et Architecture Web
http://www.jolicode.com/

Derick Rethans

2 years ago

On 5 April 2024 15:09:42 CEST, Joel Wurtz <jwurtz@jolicode.com> wrote:

>Hello everyone,
>
>Like a lot of libraries, we offer the possibility to configure behaviors
>with Attributes. However in some cases it's wrongly configured by the user
>and this wrong configuration cannot be detected on the attribute
>constructor but afterwards.
>
>In this case we may want to pinpoint which attribute (in which file and at
>which line) cause this bad configuration. Since there was no method to
>retrieve those information in the ReflectionAttribute I proposed a PR
>https://github.com/php/php-src/pull/13889 to add those informations.
>
>I do believe this will allow better DX for end user when correctly used,

Would it make sense to not only add this for ReflectionAttribute, but also Function and/or others?

Functions have a range of lines associate with them. Could Attributes have that too?

cheers
Derick

Larry Garfield

2 years ago

On Fri, Apr 5, 2024, at 1:09 PM, Joel Wurtz wrote:

> Hello everyone,
>
> Like a lot of libraries, we offer the possibility to configure
> behaviors with Attributes. However in some cases it's wrongly
> configured by the user and this wrong configuration cannot be detected
> on the attribute constructor but afterwards.
>
> In this case we may want to pinpoint which attribute (in which file and
> at which line) cause this bad configuration. Since there was no method
> to retrieve those information in the ReflectionAttribute I proposed a
> PR https://github.com/php/php-src/pull/13889 to add those informations.
>
> I do believe this will allow better DX for end user when correctly used,
>
> Regards,
>
> Joël Wurtz

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.)

--Larry Garfield

Ilija Tovilo

2 years ago

Hi Joel

On Fri, Apr 5, 2024 at 3:10 PM Joel Wurtz <jwurtz@jolicode.com> wrote:

>
> Like a lot of libraries, we offer the possibility to configure behaviors with Attributes. However in some cases it's wrongly configured by the user and this wrong configuration cannot be detected on the attribute constructor but afterwards.
>
> In this case we may want to pinpoint which attribute (in which file and at which line) cause this bad configuration. Since there was no method to retrieve those information in the ReflectionAttribute I proposed a PR https://github.com/php/php-src/pull/13889 to add those informations.
>
> I do believe this will allow better DX for end user when correctly used,

I would propose negating and renaming isUserDefined() to isInternal(),
since we already have several such methods. As hinted in the PR, I
believe the implementation is wrong, or rather doesn't do what you
would expect it to do.

I'm also wondering whether it may be more useful to expose the
attribute class as getClass(), without instantiating the attribute
itself. This would allow you to check for isInternal() there, along
with all the other class reflection information.

Ilija

Joel Wurtz

2 years ago

> 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ël

Larry Garfield

2 years ago

On Fri, Apr 5, 2024, at 2:20 PM, Joel Wurtz wrote:

>> 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.

OK, so the idea is you could do something like:

$rClass = new ReflectionClass(Entity::class);
$as = $rClass->getAttributes();
$attribs = array_map(fn(ReflectionAttribute $r) => $r->getInstance(), $as);
$values = array_map(fn(CustomAttribute $r) => $r->foo, $attribs);

if ($values is not unique) {
$msg = sprintf('You have too many %s on in file %s on line %d', Entity::class, $as[0]->getFile(), $as[0]->getLine());
throw new Exception($msg);
}

(Or something like that.)

That works, I suppose, but couldn't that also be done with ReflectionClass::getStartLine() and ReflectionClass::getFileName()?

--Larry Garfield

Joel Wurtz

2 years ago

Le ven. 5 avr. 2024 à 18:04, Larry Garfield <larry@garfieldtech.com> a
écrit :

> On Fri, Apr 5, 2024, at 2:20 PM, Joel Wurtz wrote:
> >> 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.
>
> OK, so the idea is you could do something like:
>
> $rClass = new ReflectionClass(Entity::class);
> $as = $rClass->getAttributes();
> $attribs = array_map(fn(ReflectionAttribute $r) => $r->getInstance(), $as);
> $values = array_map(fn(CustomAttribute $r) => $r->foo, $attribs);
>
> if ($values is not unique) {
>   $msg = sprintf('You have too many %s on in file %s on line %d',
> Entity::class, $as[0]->getFile(), $as[0]->getLine());
>   throw new Exception($msg);
> }
>
> (Or something like that.)
>
> That works, I suppose, but couldn't that also be done with
> ReflectionClass::getStartLine() and ReflectionClass::getFileName()?
>
> --Larry Garfield
>

Yes, it could work for a class, but not for a ReflectionProperty or
ReflectionParameter also we only have the line for the start of the class,
not the line of the attribute, in case where there is a lot of attributes
(i.e doctrine entity in a symfony project with validation , serializer, api
resource, etc ....) being able to have the correct line is better (even
more if you have repeated attributes, and one work but not the other)

I'm wondering if this would be useful to also have the offset when there is
multiple attribute on the same line (like attribute for ReflectionParameter)

Joel