Implement GH-12908: Show attribute name/class in ReflectionAttribute … #12917
Conversation
…te dump This is consistent with how many other Reflection classes have a name field, and it makes debugging easier.
| @@ -764,6 +764,8 @@ class ReflectionAttribute implements Reflector | |||
| /** @cvalue REFLECTION_ATTRIBUTE_IS_INSTANCEOF */ | |||
| public const int IS_INSTANCEOF = UNKNOWN; | |||
|
|
|||
| public string $name; | |||
There was a problem hiding this comment.
Perhaps, although the others aren't. The readonly-ness is handled manually by _reflection_write_property and it throws a ReflectionException. So for consistency I went this route.
The readonly modifier makes it throw an Error, so changing the behaviour of the others would be a small BC break.
There was a problem hiding this comment.
I also think this should be possible because changing the property makes zero sense, and makes no difference. Some people may naively think that the name property has an effect on the attribute itself.
TBH we also made a few properties readonly in the past, so I think it's doable now as well, unless some very popular library (e.g. BetterReflection) redefine them.
There was a problem hiding this comment.
OK, I'll check BetterReflection some time later and if that's okay I'll drop the special handling and let these use readonly
There was a problem hiding this comment.
Here is the patch to remove the special handling: https://gist.github.com/nielsdos/e69d6b826fecf60a7c3fedce3f83ff7f
However, we can't apply that patch. BetterReflection relies on the behaviour (e.g. Roave/BetterReflection@3eedf48 but there are more cases, also exception classes in catch would be required to change) and that patch therefore causes 562 test failures.
| @@ -1122,6 +1122,7 @@ static void reflection_attribute_factory(zval *object, HashTable *attributes, ze | |||
| reference->target = target; | |||
| intern->ptr = reference; | |||
| intern->ref_type = REF_TYPE_ATTRIBUTE; | |||
| ZVAL_STR_COPY(reflection_prop_name(object), data->name); | |||
There was a problem hiding this comment.
why is this needed and not in b0e83aa?
There was a problem hiding this comment.
Because there is already code that copies into the property table for the case you linked.
There was a problem hiding this comment.
My commit got reverted, I missed the fact that the property is inherited from ReflectionFunctionAbstract.
This PR was merged into the 5.4 branch. Discussion ---------- [VarDumper] Fix test suite with PHP 8.4 | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | Fix #54180 | License | MIT Lately, attribute name has been added to `ReflectionAttribute`, which makes messed up with the CI a bit with PHP 8.4. Here is the PR: php/php-src#12917 We can change the current virtual dump property to some "real" one by adding `name` in the caster when it doesn't exist yet. If it already exists, we're running PHP 8.4 or later. This way we avoid having different tests with the different PHP versions. Should we add something in `ReflectionCaster` to remember to remove this part when the minimal PHP version Symfony requires is >= 8.4 ? If yes, what would be the best "syntax"? Commits ------- 94426aa [VarDumper] Fix test suite with PHP 8.4
…dump
This is consistent with how many other Reflection classes have a name field, and it makes debugging easier.