olvlvl / composer-attribute-collector

A convenient and near zero-cost way to retrieve targets of PHP 8 attributes
Other
152 stars 3 forks source link

Catch exceptions when creating attributes #9

Closed withinboredom closed 1 year ago

withinboredom commented 1 year ago

I had a cryptic error:

Unknown named parameter $Method at /var/www/vendor/olvlvl/composer-attribute-collector/src/Collection.php:63)

I didn't know which attribute it was referring to. After adding this line, when it failed to create an attribute, I was able to get a more useful error message/trace.

This should also be more helpful when tracking down errors inside constructors as well.

olvlvl commented 1 year ago

Hi Rob, thanks for the PR. Can you give me more context here? How come decorating an exception provides more context? Don't you get a stack trace when you run composer dump -vvv? Also, how come the plugin pickup the wrong parameter name? Was it wrong in the source?

withinboredom commented 1 year ago

Can you give me more context here? How come decorating an exception provides more context? Don't you get a stack trace when you run composer dump -vvv? Also, how come the plugin pickup the wrong parameter name? Was it wrong in the source?

The best is to show an example:

<?php
#[Attribute]
class Test {
  public function __construct(int $number) {}
}

function Example(#[Test(Number: 5)] string $var): void {}

Technically, there isn't an error here (as PHP is case-insensitive). However, Number != number and when using array-unpacking, there will be an error. This error will just be "unknown named parameter $Number" ... or you could accidentally pass a string instead of an int (which static analysis will catch, FWIW).

The error is unclear because what "Number", on what method? Worse, this error appears at runtime, not during dumping. The stack trace does indeed show it coming from this function, but no other information is given as to what class/attribute is being instantiated since this is done as part of a loop and thus not part of the stack.

By decorating the exception, we can see more details about the error which can make tracking down the actual location of the error easier.

olvlvl commented 1 year ago

@withinboredom Thanks for the explanation, if it helps understand the nature of the error it could be a good thing to have. Could you please write tests for it?

Also, I'm wondering if that counts as a bug in PHP. If the case doesn't matter in general, how come it matters when unpacking arguments?

withinboredom commented 1 year ago

Could you please write tests for it?

I'm not sure what value the tests would have, we'd be basically testing that PHP exceptions work which is hopefully already tested by PHP itself.

If the case doesn't matter in general, how come it matters when unpacking arguments?

Today I learned PHP variable names are, in fact, case-sensitive. I had never actually tested that.

php > $a = 1;
php > $A = 2;
php > echo $a;
1
php > echo $A;
2
olvlvl commented 1 year ago

@withinboredom I opened a PR over here: https://github.com/olvlvl/composer-attribute-collector/pull/11, wdyt?

olvlvl commented 1 year ago

https://github.com/olvlvl/composer-attribute-collector/pull/11