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

Add a ReflectionPropertyAccessor #716

Merged
merged 1 commit into from
Apr 15, 2017

Conversation

ogizanagi
Copy link
Contributor

I use this accessor mainly for hydratation because my domain objects do not have proper setters for every properties, or in order to set predictable values for those computed directly in the constructor.
However, I wonder if it should be wired by default or not.

Copy link
Member

@theofidry theofidry left a comment

Choose a reason for hiding this comment

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

I'm ok to add this accessor as it can be useful, I however wouldn't wire it by default. I actually removed that behaviour in 3.x (setting properties by accessing them directly) because it was causing too many issues

It would also be nice to add a doc entry for it as well :)


class DummyWithPrivateProperty
{
private $val;
Copy link
Member

Choose a reason for hiding this comment

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

you should add a test with a private static property

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean to ensure the accessor never consider static properties? Done (and accessor updated).

throw $e;
}

$getPropertyClosure = \Closure::bind(function ($object) use ($propertyPath) {
Copy link
Member

Choose a reason for hiding this comment

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

(CS)

$getPropertyClosure = \Closure::bind(
    function ($object) use ($propertyPath) {
        return $object->{$propertyPath};
    },
    $objectOrArray,
    $objectOrArray
);

$class = get_class($objectOrArray);
$reflClass = new \ReflectionClass($class);

return $reflClass->hasProperty($propertyPath);
Copy link
Member

Choose a reason for hiding this comment

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

return (new \ReflectionClass($class))->hasProperty($propertyPath);

(I would keep the $class though)

@@ -13,6 +13,7 @@

namespace Nelmio\Alice\Loader;

use Nelmio\Alice\Entity\DummyWithPrivateProperty;
Copy link
Member

Choose a reason for hiding this comment

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

hm, the order doesn't seem right, I really need to add a StyleCI...

@ogizanagi
Copy link
Contributor Author

Changes made. :)

Where would the documentation fit the best in the current documentation? (mention the decoration of the loader property accessor + symfony configuration sample)

@theofidry
Copy link
Member

I would say here and maybe mentioning it there as well:

You can reference properties reachable through a getter (i.e : @name->property will call $name->getProperty() if property is not public)

* @expectedException \Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException
* @expectedExceptionMessage Cannot read property "staticVal".
*/
public function testThrowsAnOriginalExceptionIfPropertyIsStatic()
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be able to read it?

Copy link
Contributor Author

@ogizanagi ogizanagi Apr 15, 2017

Choose a reason for hiding this comment

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

I don't think we should support it (use cases look a bit edgy to me), but you set the rules. 😄

Actually, I'm not even sure the property access component is meant to be used to access any static property.

@ogizanagi
Copy link
Contributor Author

Documentation added.

@theofidry theofidry merged commit bcbbff1 into nelmio:master Apr 15, 2017
@ogizanagi ogizanagi deleted the feature/refl_accessor branch April 15, 2017 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants