Implemented ReflectionProperty::setAccessible(), getValue() and setValue() and ReflectionClass::getStaticProperties() by kukulich · Pull Request #343 · Roave/BetterReflection

kukulich · 2017-08-31T22:08:15Z

No description provided.

Ocramius · 2017-09-01T17:18:57Z

src/Reflection/ReflectionClass.php

-        // reasoning...
-        if ( ! $property->isPublic()) {
-            throw new PropertyNotPublic('Property is not public');
+            throw PropertyDoesNotExistOrIsNotStatic::fromName($propertyName);


You know if it does not exist or if it is static, so throwing a PropertyDoesNotExistOrIsNotStatic feels wrong.

A better way to handle this is having PropertyDoesNotExistOrIsNotStatic be an abstract class, and then have two subtypes for the actual failure: that would make it much easier to deal with for a consumer, and would read more sensibly.

Ok. I just used the code that was already here :) https://github.com/Roave/BetterReflection/blob/master/src/Reflection/ReflectionClass.php#L1309

Aye - I know my comments sometimes sound annoying and out of context, but I really just see the diffs only ^_^

It's ok :) I hope I will someday prepare a pull request that can be merged straight away :)

Ocramius · 2017-09-01T17:19:49Z

src/Reflection/Adapter/ReflectionClass.php

+            if (2 === \func_num_args()) {
+                return $default;
+            }
+            throw new CoreReflectionException($e->getMessage());


Spacing please: spaces around logical blocks :-)

Shouldn't we populate the previous exception?

Ocramius · 2017-09-01T17:20:03Z

src/Reflection/Adapter/ReflectionClass.php

+                return $default;
+            }
+            throw new CoreReflectionException($e->getMessage());
+        } catch (Throwable $e) {


When can this happen?

Ocramius · 2017-09-01T17:20:13Z

src/Reflection/Adapter/ReflectionClass.php

+            }
+            throw new CoreReflectionException($e->getMessage());
+        } catch (Throwable $e) {
+            throw new CoreReflectionException($e->getMessage());


Shouldn't we populate the previous exception?

Ocramius · 2017-09-01T17:20:23Z

src/Reflection/Adapter/ReflectionClass.php

+        try {
+            $this->betterReflectionClass->setStaticPropertyValue($name, $value);
+        } catch (Throwable $e) {
+            throw new CoreReflectionException($e->getMessage());


Shouldn't we populate the previous exception?

Ocramius · 2017-09-01T17:22:02Z

src/Reflection/Adapter/ReflectionProperty.php

+        } catch (NoObjectProvided | NotAnObject $e) {
+            return null;
+        } catch (Throwable $e) {
+            throw new CoreReflectionException($e->getMessage());


Shouldn't we populate the previous exception?

Ocramius · 2017-09-01T17:23:32Z

src/Reflection/ReflectionProperty.php

+    {
+        $this->assertAccessibility();
+
+        $declaringClassName = $this->getDeclaringClass()->getName();


Scenario to check: https://3v4l.org/8fBDe

Ocramius · 2017-09-01T17:24:07Z

src/Reflection/ReflectionProperty.php

+     */
+    public function getValue($object = null)
+    {
+        $this->assertAccessibility();


IMO, our own implementation should drop the setAccessible() bullcrap

Done. setAccessible() implemented only in adapters.

Ocramius · 2017-09-01T17:24:38Z

src/Reflection/ReflectionProperty.php

+            $this->assertClassExist($declaringClassName);
+
+            return Closure::bind(function (string $propertyName) {
+                return self::${$propertyName};


Instead of self::, use $declaringClassName

Ocramius · 2017-09-01T17:25:03Z

src/Reflection/ReflectionProperty.php

+        $this->assertObject($object);
+
+        return Closure::bind(function (string $propertyName) {
+            return $this->{$propertyName};


Instead of $this, use a parameter $object and call the closure with that

(this is to confuse the static analysers less

…lue()

kukulich · 2017-09-01T19:55:25Z

@Ocramius I hope everything is ok now :)

…ties` to `getStaticPropertiesValues`

…icProperties` to `getStaticPropertiesValues`" This reverts commit 9e82778.

Close #343

Ocramius · 2017-09-02T09:29:09Z

@kukulich merged, thanks! I tried to improve something in 9e82778, but failed due to how the test is (reasonably) designed.

🚢 and thanks!

kukulich · 2017-09-02T10:34:11Z

@Ocramius I'm only on mobile now but if I understand it well then remove this line https://github.com/Roave/BetterReflection/blob/master/test/unit/Reflection/Adapter/ReflectionClassTest.php#L90 and implement special test for "getStaticProperties". Some other methods are already tested in similar way.

Ocramius · 2017-09-02T10:35:46Z

That's alright: the naming is not the end of the world here, as it would probably be more confusing to have a different one Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/

…

On Sat, Sep 2, 2017 at 12:34 PM, Jaroslav Hanslík ***@***.***> wrote: @Ocramius <https://github.com/ocramius> I'm only on mobile now but if I understand it well then remove this line https://github.com/Roave/ BetterReflection/blob/master/test/unit/Reflection/Adapter/ ReflectionClassTest.php#L90 and implement special test for "getStaticProperties". Some other methods are already tested in similar way. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#343 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakCxBpzQnWTZqMR9bCnu3pAKaZBjAks5seS8jgaJpZM4PJeHM> .

kukulich force-pushed the property-value branch 4 times, most recently from 8b54e22 to 032bf83 Compare September 1, 2017 08:55

kukulich changed the title ~~Implemented ReflectionProperty::setAccessible(), getValue() and setValue()~~ Implemented ReflectionProperty::setAccessible(), getValue() and setValue() and ReflectionClass::getStaticProperties() Sep 1, 2017

kukulich force-pushed the property-value branch from 032bf83 to e181347 Compare September 1, 2017 09:04

kukulich mentioned this pull request Sep 1, 2017

Implement accessors without instantiating #14

Closed

11 tasks

Ocramius requested changes Sep 1, 2017

View reviewed changes

Ocramius assigned kukulich Sep 1, 2017

Ocramius added enhancement BC break labels Sep 1, 2017

Ocramius added this to the 2.0.0 milestone Sep 1, 2017

kukulich force-pushed the property-value branch from e181347 to c7e70d2 Compare September 1, 2017 19:47

kukulich added 2 commits September 1, 2017 21:54

Implemented ReflectionProperty::setAccessible(), getValue() and setVa…

4be2f3d

…lue()

Implemented ReflectionClass::getStaticProperties()

f3a5103

kukulich force-pushed the property-value branch from c7e70d2 to f3a5103 Compare September 1, 2017 19:54

Ocramius merged commit f3a5103 into Roave:master Sep 2, 2017

Ocramius added a commit that referenced this pull request Sep 2, 2017

#343 at least in the better-reflection API, renaming `getStaticProper…

9e82778

…ties` to `getStaticPropertiesValues`

Ocramius added a commit that referenced this pull request Sep 2, 2017

Revert "#343 at least in the better-reflection API, renaming `getStat…

3794aba

…icProperties` to `getStaticPropertiesValues`" This reverts commit 9e82778.

Ocramius added a commit that referenced this pull request Sep 2, 2017

#343 minor CS fixes (spacing)

8bec344

Ocramius added a commit that referenced this pull request Sep 2, 2017

Merge branch 'feature/#343-property-read-and-write'

9f5aef3

Close #343

Ocramius assigned Ocramius and unassigned kukulich Sep 2, 2017

kukulich deleted the property-value branch September 2, 2017 10:34

Ocramius mentioned this pull request Sep 8, 2017

Should setStaticPropertyValue trigger autoloading? #265

Closed

Conversation

kukulich commented Aug 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kukulich commented Sep 1, 2017

Uh oh!

Ocramius commented Sep 2, 2017

Uh oh!

kukulich commented Sep 2, 2017

Uh oh!

Ocramius commented Sep 2, 2017 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants