[PHP 7.1] Add support for ReflectionClassConstant (#281) #282

POPSuL · 2017-06-07T13:55:56Z

Note: I did not understand how testReflectionObjectOverridesAllMethodsInReflectionClass works, therefore I missed it.

Ocramius

String cast and static export aren't covered by tests

Ocramius · 2017-06-07T14:52:33Z

src/Reflection/ReflectionClass.php


 class ReflectionClass implements Reflection, \Reflector
 {
+


Ocramius · 2017-06-07T14:53:29Z

src/Reflection/ReflectionClass.php

+        $constants = $this->getReflectionConstants();
+
+        if (!isset($constants[$name])) {
+            return null;


I'd rather have an exception here. null makes it a silent failure

In original php-reflection this method returns false:
https://github.com/php-src/php/blob/master/ext/reflection/php_reflection.c#L4547

Urgh, that's just horrible :-(

Ocramius · 2017-06-07T14:53:56Z

src/Reflection/ReflectionClass.php

+    {
+        $constants = $this->getReflectionConstants();
+
+        if (!isset($constants[$name])) {


array_key_exists()

Ocramius · 2017-06-07T14:54:45Z

src/Reflection/ReflectionClass.php

+
+        $constants = [];
+
+        foreach ($this->node->stmts as $stmt) {


Use an array filter here. A private method where you do it would be best

Ocramius · 2017-06-07T14:55:24Z

src/Reflection/ReflectionClass.php

+            }
+        }
+
+        $this->cachedReflectionConstants = $constants;


Can return directly on this assignment

Ocramius · 2017-06-07T15:00:46Z

src/Reflection/ReflectionClassConstant.php

+     *
+     * @return bool
+     */
+    public function isProtected()


Return hint. Docblock is redundant

Would you be for adding coding standard for these?

If a return type can be specified, it must be specified

I agree! There is a sniff for that that can check (and complete) that for you.

Ocramius · 2017-06-07T15:01:21Z

src/Reflection/ReflectionClassConstant.php

+    {
+        if ($this->isPublic()) {
+            return '<public>';
+        } elseif ($this->isPrivate()) {


else not needed

Ocramius · 2017-06-07T15:01:31Z

src/Reflection/ReflectionClassConstant.php

+            return '<public>';
+        } elseif ($this->isPrivate()) {
+            return '<private>';
+        } elseif ($this->isProtected()) {


Ocramius · 2017-06-07T15:01:48Z

src/Reflection/ReflectionClassConstant.php

+        } elseif ($this->isProtected()) {
+            return '<protected>';
+        }
+        return '<unknown>';


Why the brackets?

Ocramius · 2017-06-07T15:02:44Z

test/unit/Reflection/ReflectionClassConstantTest.php

+{
+    private function getComposerLocator() : ComposerSourceLocator
+    {
+        global $loader;


Really smelly...

Yep, is really smelly code, but...
https://github.com/Roave/BetterReflection/blob/master/test/unit/Reflection/ReflectionClassTest.php#L44 :(

Can you try $loader = require __DIR__ . '/../../../lots-of-../../vendor/autoload.php';?

Ocramius · 2017-06-07T20:54:21Z

Note: I did not understand how testReflectionObjectOverridesAllMethodsInReflectionClass works, therefore I missed it.

It's just a test that checks that all method calls go through to an underlying better-reflection class. It's just there for API compatibility: if php-src defines new reflection methods, this test catches them.

POPSuL · 2017-06-08T09:04:55Z

Also need:

::getModifiers() : int;
::getDeclaringClass() : ReflectionClass
::getDocComment() : ?string

I'll add it later

POPSuL · 2017-06-08T11:40:33Z

Huh, I think I'm done.

Ocramius · 2017-06-08T17:37:46Z

src/Reflection/ReflectionClass.php

+    public function getReflectionConstants() : array
+    {
+        if (null === $this->cachedReflectionConstants) {
+            $this->cachedReflectionConstants = array_reduce(


Just noticed that this code does not consider parent classes and traits - anything that we can do there?

@Ocramius native reflection not provide access for constants inherited from parents. Current behavior is exactly the same as in the native reflection.

class A { const A = 1; } class B { const B = 2; } foreach ([A::class, B::class] as $class) { echo 'Native ', $class, ':', PHP_EOL; $ref = new \ReflectionClass($class); foreach ($ref->getReflectionConstants() as $const) { echo $const; } echo 'Better ', $class, ':', PHP_EOL; $ref = $reflector->reflect($class); foreach ($ref->getReflectionConstants() as $const) { echo $const; } }

Output:

Native A: Constant [ public integer A ] { 1 } Better A: Constant [ public integer A ] { 1 } Native B: Constant [ public integer B ] { 2 } Better B: Constant [ public integer B ] { 2 }

And traits doesn't support constants.

@Ocramius native reflection not provide access for constants inherited from parents. Current behavior is exactly the same as in the native reflection.

We should provide a "clean" implementation in our own code. For instance, we have Roave\BetterReflection\Reflection\ReflectionClass#getImmediateMethods() for methods implemented only at the current inheritance level (without considering ancestors), and then we provide a reflection-compatible API that retrieves all methods, exactly like internal reflection. The same should apply here.

And traits doesn't support constants.

Didn't know this, thanks for clarifying!

@Ocramius I added new methods and class to reflection-compat API.
But new features for class constants is not related to this PR, IMHO.

But new features for class constants is not related to this PR, IMHO.

It's just in line with the package style: it's Better Reflection, not just reflection ;-)

TomasVotruba · 2017-06-26T12:34:13Z

@Ocramius We depend on this PR in ApiGen and after 2 weeks there is probably something missing. What needs to be done here?

Ocramius · 2017-06-26T12:36:00Z

@TomasVotruba I simply didn't see the pushes. A rebase is needed, then another round of reviews

TomasVotruba · 2017-06-26T12:39:23Z

@Ocramius Allright, thanks for update.

@POPSuL Could you look on it please?

POPSuL · 2017-06-26T13:17:31Z

Hmmm... Strange conflict... I check this tomorrow.

ReflectionClass::getReflectionConstants() was refactored

…::getComposerLocator()

…Node() Added ::getModifiers(), ::getDeclaringClass() and ::getDocComment()

…tring()

POPSuL · 2017-06-29T10:18:24Z

@Ocramius rebased 2 days ago ;)

Ocramius · 2017-06-29T10:21:34Z

src/Reflection/ReflectionClassConstant.php

+     */
+    public function getDocComment() : string
+    {
+        if (!$this->node->hasAttribute('comments')) {


Please see #284 about the contents of this method

Ocramius · 2017-06-29T10:21:57Z

src/Reflection/Adapter/ReflectionClassConstant.php

+use \ReflectionClassConstant as CoreReflectionClassConstant;
+use Roave\BetterReflection\Reflection\ReflectionClassConstant as BetterReflectionClassConstant;
+
+class ReflectionClassConstant extends CoreReflectionClassConstant


Ocramius · 2017-06-29T10:22:07Z

src/Reflection/Adapter/ReflectionClassConstant.php

+    {
+        $this->betterClassConstant = $betterClassConstant;
+    }
+


Empty line to be removed

Ocramius · 2017-06-29T10:22:09Z

src/Reflection/Adapter/ReflectionClassConstant.php

+
+class ReflectionClassConstant extends CoreReflectionClassConstant
+{
+    private $betterClassConstant;


Ocramius · 2017-06-29T10:23:01Z

src/Reflection/ReflectionClass.php

+            return null;
+        }
+
+        return $constants[$name];


return $this->getReflectionConstants()[$name] ?? null

Ocramius · 2017-06-29T10:26:31Z

src/Reflection/ReflectionClassConstant.php

+    private $valueCached = false;
+
+    /**
+     * @var mixed const value


Ocramius · 2017-06-29T10:27:15Z

src/Reflection/ReflectionClassConstant.php

+     */
+    public function getValue()
+    {
+        if (false === $this->valueCached) {


Consider flipping over the conditional, so the processing comes after an early return

Ocramius · 2017-06-29T10:27:51Z

src/Reflection/ReflectionClassConstant.php

+     */
+    public function getModifiers() : int
+    {
+        return $this->node->flags === 0 ? Class_::MODIFIER_PUBLIC : $this->node->flags;


Is 0 considered public at all times?

Maybe we should store the correct modifier if no modifier is defined (public) instead? How much does that go against the original reflection API?

If modifier not provided (just const A = 'b') visibility is public.
Original behavior I'll check tomorrow.

Ocramius · 2017-06-29T10:30:19Z

src/Reflection/ReflectionClassConstant.php

+     */
+    public function getDeclaringClass() : ReflectionClass
+    {
+        return $this->owner;


What about non-class constants?

<?php namespace Foo { const BAR = 'baz'; } namespace { var_dump(\Foo\BAR); }

https://3v4l.org/C0hEF

PHP doesn't have reflection for constants.
ReflectionClassConstant can be instantiated only on ReflectionClass and only for constant related to "this" class.

This was further discussed later on. Indeed, further work on non-class constants will not land in this patch 👍

Ocramius · 2017-06-29T10:30:33Z

src/Reflection/ReflectionClassConstant.php

+        if ($this->isProtected()) {
+            return 'protected';
+        }
+        return '';


Should we even consider this scenario?

…ve reflection behavior

…return `public` as default

…ion#282

POPSuL · 2017-07-14T13:47:16Z

+1 week... :(

Ocramius · 2017-07-14T19:35:10Z

@POPSuL patience - you're lucky it's not one of the "3 year old PRs" in my backlog ;-)

@asgrim did we decide something about namespaced constants?

namespace Foo {
    const BAR = 'baz';
}

asgrim · 2017-07-18T12:05:42Z

@Ocramius this is specifically to do with class constants, so not worried about defined or namespaced constants here I think.

My onl concern here is if a new API is introduced into PHP that allows reflection of constants (whether specific to class constants like this) or even all "forms" of constants. This API would then conflict with this, so if that were the case, we'd need to release a new major as BC would be broken. Apart from that 👍 from me.

asgrim · 2017-07-18T12:06:40Z

test/unit/Fixture/ExampleClassExport.txt

-    Constant [ integer MY_CONST_1 ] { 123 }
-    Constant [ integer MY_CONST_2 ] { 234 }
+  - Constants [5] {
+    Constant [ public integer MY_CONST_1 ] { 123 }


Does this information exist when exporting a reflection in core PHP?

asgrim · 2017-07-18T12:10:39Z

src/Reflection/ReflectionClass.php

+
+        return $this->cachedReflectionConstants = array_reduce(
+            $this->node->stmts,
+            [$this, 'processReflectionConstants'],


This method can just be inlined here I reckon, it's only used the once and I can't see it being re-usable?

Ocramius · 2017-07-18T21:59:01Z

My onl concern here is if a new API is introduced into PHP that allows reflection of constants (whether specific to class constants like this) or even all "forms" of constants.

We would simply NOT release that part as a Roave\BetterReflection\Reflection\Adapter\* class (that's the bit that is affected by BC issues if PHP-SRC releases new reflection functionality)

asgrim · 2017-07-18T22:05:44Z

Actually, that's a good point. The Adapter class for this is totally unnecessary at this time :)

Ocramius · 2017-07-18T22:09:40Z

@asgrim can you create an issue for the namespaced (non-class) constant then? Not to be tackled here at this point.

Ocramius

LGTM 👍

Thanks @POPSuL! I'm merging after applying a few nitpicks :-)

Ocramius · 2017-07-18T22:12:34Z

src/Reflection/ReflectionClass.php

+     *
+     * @return ReflectionClassConstant[]
+     */
+    public function getReflectionConstants() : array


Note to self: this block is to be changed at merge time (not a reduce operation)

Ocramius · 2017-07-18T22:14:21Z

src/Reflection/ReflectionClassConstant.php

+     */
+    public function getDeclaringClass() : ReflectionClass
+    {
+        return $this->owner;


This was further discussed later on. Indeed, further work on non-class constants will not land in this patch 👍

Ocramius · 2017-07-18T22:14:59Z

src/Reflection/ReflectionClassConstant.php

+     *
+     * @link http://php.net/manual/en/reflector.export.php
+     * @return string
+     * @since 5.0


Note to self: fix comment on merge

Ocramius · 2017-07-18T22:15:09Z

src/Reflection/ReflectionClassConstant.php

+     *
+     * @link http://php.net/manual/en/reflector.tostring.php
+     * @return string
+     * @since 5.0


Note to self: fix comment on merge

Ocramius · 2017-07-18T22:28:11Z

Just referencing it for confused folks: ReflectionClassConstant exists - see https://bugs.php.net/bug.php?id=74261

asgrim · 2017-07-18T22:32:24Z

Ah yes, confused me indeed. Couldn't find documentation, so disregard my earlier comment about removing the Adapter. Does the API definitely match? How do we verify compatibility without checking the src?

Ocramius · 2017-07-18T22:35:52Z

@asgrim there is a ReflectionClassConstantTest - I'm verifying it.

…array_map` and `array_combine` sequence

… extended\)

Close #282 he commit.

Ocramius · 2017-07-18T22:45:19Z

Fixed the nitpicks and merged into master at 44fdc0a, thanks @POPSuL!

* [WIP] Initial support for ReflectionConstant from Roave/BetterReflection#282 * [WIP] Added descriptions and deprecations for constants * [WIP] Fixed broken tests * Added constants to stub * Fixed ClassConstantReflection::getAnnotations() and cs * Fixed tests and cs * Implemented getStart/EndLine for class constants and properties

POPSuL mentioned this pull request Jun 7, 2017

Class constants not supported ApiGen/ApiGen#877

Closed

Ocramius requested changes Jun 7, 2017

View reviewed changes

Ocramius added enhancement WIP labels Jun 7, 2017

Ocramius requested changes Jun 8, 2017

View reviewed changes

POPSuL added 12 commits June 27, 2017 10:21

rebased to master

b319931

[WIP] added some docblocks and return type hints

9526274

ReflectionClass::getReflectionConstants() was refactored

[WIP] Fixed testReflectionObjectOverridesAllMethodsInReflectionClass

26eb03e

[WIP] Removed usage of global variable in ReflectionClassConstantTest…

fb7b776

…::getComposerLocator()

[WIP] Fixed ReflectionClassConstant::__toString() format

5b605ac

[WIP] ::createFromConstFlagsNameAndValue() replaced with ::createFrom…

85ae1da

…Node() Added ::getModifiers(), ::getDeclaringClass() and ::getDocComment()

[WIP] Added type of constant value for ReflectionClassConstant::__toS…

87bd84c

…tring()

[WIP] Added constant visibility for ReflectionClass::__toString()

c33b5cc

[Tests] ::assertEquals() replaced with ::assertSame()

e740b4d

Fixed value caching flag for class constants

a8c0c0d

Added ReflectionClassConstant to compat-api

421efbd

Removed unnecessary import

2ee8f98

POPSuL force-pushed the issue/php7-const-visibility branch from 9a7de41 to 2ee8f98 Compare June 26, 2017 23:26

Ocramius requested changes Jun 29, 2017

View reviewed changes

POPSuL added 3 commits June 30, 2017 01:20

Various code style fixes

5c40e4e

Fixed ReflectionClassConstant::getModifiers() compatibility with nati…

a969286

…ve reflection behavior

Fixed ReflectionClassConstant::getVisibility() behavior - now always …

e1eee2c

…return `public` as default

POPSuL added a commit to POPSuL/ApiGen that referenced this pull request Jul 3, 2017

[WIP] Initial support for ReflectionConstant from Roave/BetterReflect…

8d664ef

…ion#282

POPSuL mentioned this pull request Jul 3, 2017

[WIP] Initial support for ReflectionConstant ApiGen/ApiGen#926

Merged

asgrim requested changes Jul 18, 2017

View reviewed changes

Inlined ReflectionClass::processReflectionConstants()

926a5f6

Ocramius approved these changes Jul 18, 2017

View reviewed changes

Ocramius removed the WIP label Jul 18, 2017

Ocramius self-assigned this Jul 18, 2017

Ocramius added this to the 2.0.0 milestone Jul 18, 2017

Ocramius merged commit 926a5f6 into Roave:master Jul 18, 2017

Ocramius added a commit that referenced this pull request Jul 18, 2017

#282 replaced confusing array_reduce call with an array_filter, `…

3f1b1ba

…array_map` and `array_combine` sequence

Ocramius added a commit that referenced this pull request Jul 18, 2017

#282 replacing inherited docblocks for clarity $internal class being…

deefc3a

… extended$

Ocramius added a commit that referenced this pull request Jul 18, 2017

#282 EOF EOL + removed unused imports

eca1c9f

Ocramius added a commit that referenced this pull request Jul 18, 2017

#282 simplified ReflectionClassConstantTest

e146765

Ocramius added a commit that referenced this pull request Jul 18, 2017

#282 simplified ReflectionClassConstantTest

67bad09

Ocramius added a commit that referenced this pull request Jul 18, 2017

Merge branch 'fix/#282-const-visibility-and-reflection-class-constant'

44fdc0a

Close #282 he commit.

Ocramius mentioned this pull request Jul 23, 2017

[PHP7.1] ::getReflectionConstant() and ::getReflectionConstants() support #281

Closed

[PHP 7.1] Add support for ReflectionClassConstant (#281) #282

[PHP 7.1] Add support for ReflectionClassConstant (#281) #282

Conversation

POPSuL commented Jun 7, 2017

Ocramius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius commented Jun 7, 2017

POPSuL commented Jun 8, 2017

POPSuL commented Jun 8, 2017

Choose a reason for hiding this comment

POPSuL Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasVotruba commented Jun 26, 2017 • edited Loading

Ocramius commented Jun 26, 2017

TomasVotruba commented Jun 26, 2017

POPSuL commented Jun 26, 2017

POPSuL commented Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

POPSuL commented Jul 14, 2017

Ocramius commented Jul 14, 2017

asgrim commented Jul 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius commented Jul 18, 2017

asgrim commented Jul 18, 2017

Ocramius commented Jul 18, 2017

Ocramius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius commented Jul 18, 2017

asgrim commented Jul 18, 2017

Ocramius commented Jul 18, 2017

Ocramius commented Jul 18, 2017

POPSuL Jun 8, 2017 •

edited

Loading

TomasVotruba commented Jun 26, 2017 •

edited

Loading

POPSuL commented Jun 29, 2017 •

edited

Loading