Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[VarDumper] Support for ReflectionAttribute #38167

Merged
merged 1 commit into from
Sep 19, 2020

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Sep 12, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets N/A
License MIT
Doc PR not needed

VarDumper currently does not understand that certain reflection objects might have attributes attached to it. Dumping a ReflectionAttribute just yields ReflectionAttribute {#4711} which is not really helpful. This PR attempts to fix this.

ReflectionAttribute {#4711
  name: "App\MyAttribute"
  arguments: array:2 [
    0 => "one"
    "extra" => "hello"
  ]
}

While working on this, I noticed that class constants (which can be reflected on since PHP 7.1) are just dumped as plain values, so I've also added a caster for ReflectionClasConstant as bonus.

The full output for the LotsOfAttributes fixture class that is included with is PR looks like this:

^ ReflectionClass {#7
  +name: "Symfony\Component\VarDumper\Tests\Fixtures\LotsOfAttributes"
  modifiers: "final"
  implements: []
  constants: array:1 [
    0 => ReflectionClassConstant {#20
      +name: "SOME_CONSTANT"
      +class: "Symfony\Component\VarDumper\Tests\Fixtures\LotsOfAttributes"
      modifiers: "public"
      value: "some value"
      attributes: array:2 [
        0 => ReflectionAttribute {#33
          name: "Symfony\Component\VarDumper\Tests\Fixtures\RepeatableAttribute"
          arguments: array:1 [
            0 => "one"
          ]
        }
        1 => ReflectionAttribute {#34
          name: "Symfony\Component\VarDumper\Tests\Fixtures\RepeatableAttribute"
          arguments: array:1 [
            0 => "two"
          ]
        }
      ]
    }
  ]
  properties: array:1 [
    "someProperty" => ReflectionProperty {#19
      +name: "someProperty"
      +class: "Symfony\Component\VarDumper\Tests\Fixtures\LotsOfAttributes"
      modifiers: "private"
      attributes: array:1 [
        0 => ReflectionAttribute {#30
          name: "Symfony\Component\VarDumper\Tests\Fixtures\MyAttribute"
          arguments: array:2 [
            0 => "one"
            "extra" => "hello"
          ]
        }
      ]
    }
  ]
  methods: array:1 [
    "someMethod" => ReflectionMethod {#21
      +name: "someMethod"
      +class: "Symfony\Component\VarDumper\Tests\Fixtures\LotsOfAttributes"
      returnType: "void"
      parameters: {
        $someParameter: ReflectionParameter {#28
          +name: "someParameter"
          position: 0
          attributes: array:1 [
            0 => ReflectionAttribute {#42
              name: "Symfony\Component\VarDumper\Tests\Fixtures\MyAttribute"
              arguments: array:1 [
                0 => "three"
              ]
            }
          ]
          typeHint: "string"
        }
      }
      attributes: array:1 [
        0 => ReflectionAttribute {#27
          name: "Symfony\Component\VarDumper\Tests\Fixtures\MyAttribute"
          arguments: array:1 [
            0 => "two"
          ]
        }
      ]
      modifiers: "public"
    }
  ]
  attributes: array:1 [
    0 => ReflectionAttribute {#22
      name: "Symfony\Component\VarDumper\Tests\Fixtures\MyAttribute"
      arguments: []
    }
  ]
  extra: {
    file: "./src/Symfony/Component/VarDumper/Tests/Fixtures/LotsOfAttributes.php"
    line: "15 to 28"
    isUserDefined: true
  }
}

Comment on lines 361 to 364
/**
* @param \Reflector|\ReflectionAttribute $c
*/
private static function addMap(array &$a, $c, $map, $prefix = Caster::PREFIX_VIRTUAL)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 It felt a bit odd that I needed to do this, so I've opened https://bugs.php.net/bug.php?id=80097 for clarification.

@derrabus
Copy link
Member Author

The segfault that be observed in https://travis-ci.org/github/symfony/symfony/jobs/726646852 (line 3655) appeared after my changes to the ReflectionCaster class. I can reproduce it locally (php 8.0 67d21bf237 on macOS 10.15) by just executing

./phpunit src/Symfony/Component/VarDumper/Tests/Caster/ReflectionCasterTest.php

But when I rerun that command a couple of times, it eventually works and the tests pass. I haven't been able to isolate the problem yet, but maybe this is an interesting find for @nikic or @beberlei that helps with stabilizing the reflection API for attributes.

Bildschirmfoto 2020-09-12 um 22 59 17

@beberlei
Copy link
Contributor

Do you have the stacktrace for the segfault?

@nicolas-grekas nicolas-grekas added this to the next milestone Sep 14, 2020
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.
This should target master to me, as I have this rule in mind:

  • making existing code run on newest PHP versions should be considered as a bug fix
  • making Symfony compatible with new features of the language should be considered as a new feature.

@derrabus
Copy link
Member Author

I use the dump() function a lot when debugging on php 8 because I don't have a working PhpStorm/Xdebug-Setup for php 8 yet. It would be extremely helpful to be able to see attributes in the dumped output. But I understand that the policy you mentioned also applies to VarDumper, so I'm going to rebase the PR on master.

@derrabus derrabus changed the base branch from 3.4 to master September 14, 2020 09:40
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with minor comments

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with minor comments

@nicolas-grekas
Copy link
Member

for .github/patch-types.php:
src/Symfony/Component/VarDumper/Tests/Fixtures/MyAttribute.php

@fabpot
Copy link
Member

fabpot commented Sep 19, 2020

Thank you @derrabus.

@derrabus derrabus deleted the improvement/attribute-caster branch September 19, 2020 06:59
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants