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

[PropertyAccess] [RFC] Allow to get/set value on private and protected property #23938

Closed
fmata opened this Issue Aug 20, 2017 · 12 comments

Comments

Projects
None yet
5 participants
@fmata
Contributor

fmata commented Aug 20, 2017

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? yes
Symfony version 3.4

Currently in Workflow, the marking is setted using PropertyAccess but when the method is private or protected, we get an AccessException.
In a real world app, we want to restrain the calls to this method to prevent mistake but now it's impossible because Workflow relies on PropertyAccess which needs a public method.

I suggest to add a new $force parameter in PropertyAccessorInterface :

PropertyAccessorInterface::setValue(&$objectOrArray, $propertyPath, $value, $force = false)
PropertyAccessorInterface::getValue($objectOrArray, $propertyPath, $force = false)

As it's optional I think it's not a BC but I'm not sure.

WDYT ?

If the proposal is accepted, I can do the PR.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Aug 22, 2017

Member

Well, if there is no public setter for these properties, PropertyAccess will fail, because PHP forbids it to call the method.
The workflow is outside your object, so it needs to have access to a public API to update the state.

Member

stof commented Aug 22, 2017

Well, if there is no public setter for these properties, PropertyAccess will fail, because PHP forbids it to call the method.
The workflow is outside your object, so it needs to have access to a public API to update the state.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Aug 22, 2017

Member

btw, I don't understand what your $force argument would do.

Member

stof commented Aug 22, 2017

btw, I don't understand what your $force argument would do.

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Aug 22, 2017

Member

Passing $force to true would make PropertyAccessor fallbacks to reflection in case no public accessor exists, right?

Member

chalasr commented Aug 22, 2017

Passing $force to true would make PropertyAccessor fallbacks to reflection in case no public accessor exists, right?

@fmata

This comment has been minimized.

Show comment
Hide comment
@fmata

fmata Aug 22, 2017

Contributor

PropertyAccess guess the right method to call and check if it is accessible callable.

In https://github.com/symfony/symfony/blob/master/src/Symfony/Component/PropertyAccess/PropertyAccessor.php#L437 and https://github.com/symfony/symfony/blob/master/src/Symfony/Component/PropertyAccess/PropertyAccessor.php#L769 we can use reflection if $force is true to use ReflectionProperty::setAccessible().

@chalasr yep

Contributor

fmata commented Aug 22, 2017

PropertyAccess guess the right method to call and check if it is accessible callable.

In https://github.com/symfony/symfony/blob/master/src/Symfony/Component/PropertyAccess/PropertyAccessor.php#L437 and https://github.com/symfony/symfony/blob/master/src/Symfony/Component/PropertyAccess/PropertyAccessor.php#L769 we can use reflection if $force is true to use ReflectionProperty::setAccessible().

@chalasr yep

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Aug 27, 2017

Member

Why not creating your own ReflectionPropertyAccessor that'll fallback to reflection in case a decorated property accessor is unable to access a property, and inject it where you actually need this behavior? You could have a look to what we did in nelmio/alice: https://github.com/nelmio/alice/blob/master/src/PropertyAccess/ReflectionPropertyAccessor.php

Member

ogizanagi commented Aug 27, 2017

Why not creating your own ReflectionPropertyAccessor that'll fallback to reflection in case a decorated property accessor is unable to access a property, and inject it where you actually need this behavior? You could have a look to what we did in nelmio/alice: https://github.com/nelmio/alice/blob/master/src/PropertyAccess/ReflectionPropertyAccessor.php

@fmata

This comment has been minimized.

Show comment
Hide comment
@fmata

fmata Aug 27, 2017

Contributor

Thanks for the snippet but I think it may be useful to be included in core. Now I see a first usage in workflow as I said in my first post, maybe elsewhere.

Contributor

fmata commented Aug 27, 2017

Thanks for the snippet but I think it may be useful to be included in core. Now I see a first usage in workflow as I said in my first post, maybe elsewhere.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Aug 28, 2017

Member

Well, using Reflection means breaking the encapsulation of the class. This looks like a bad idea here, because PropertyAccess is meant to allow a piece of code to set a value in another object, that it does not own.
Allowing bypassing encapsulation makes the code much more fragile (as you then have to consider all your private properties as if they were public, and then you could as well make them public for real and using the existing component)

Member

stof commented Aug 28, 2017

Well, using Reflection means breaking the encapsulation of the class. This looks like a bad idea here, because PropertyAccess is meant to allow a piece of code to set a value in another object, that it does not own.
Allowing bypassing encapsulation makes the code much more fragile (as you then have to consider all your private properties as if they were public, and then you could as well make them public for real and using the existing component)

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Aug 28, 2017

Member

Btw, your argument for this change is to keep the property private to avoid mistakes in other parts of your project. But if PropertyAccess gives access to private code, other parts of your project could also be using PropertyAccess, and so reach your private fields...

Member

stof commented Aug 28, 2017

Btw, your argument for this change is to keep the property private to avoid mistakes in other parts of your project. But if PropertyAccess gives access to private code, other parts of your project could also be using PropertyAccess, and so reach your private fields...

@fmata

This comment has been minimized.

Show comment
Hide comment
@fmata

fmata Aug 28, 2017

Contributor

I agree with you it breaks the encapsulation but I can not see another way to achieve my goal although we are all aware of the limitation of the Reflection ;-)

Feel free to close if you think it's useless.

Contributor

fmata commented Aug 28, 2017

I agree with you it breaks the encapsulation but I can not see another way to achieve my goal although we are all aware of the limitation of the Reflection ;-)

Feel free to close if you think it's useless.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Aug 28, 2017

Member

@fmata your request is to make an external reusable library break encapsulation for you, just so that your own code can provide breakable encapsulation and hoping that other parts of the project don't know that there is an external library available to break this encapsulation without it being clear on review.

So this is a big -1 for doing this change in PropertyAccess.

The workflow component already makes the marking store configurable. So just build your own implementation using reflection for your project. Then, the fact that encapsulation gets broken happens only in your project (and is visible in it).
Another option is to just accept the fact that the marking store is outside the object and so needs a public API...

Member

stof commented Aug 28, 2017

@fmata your request is to make an external reusable library break encapsulation for you, just so that your own code can provide breakable encapsulation and hoping that other parts of the project don't know that there is an external library available to break this encapsulation without it being clear on review.

So this is a big -1 for doing this change in PropertyAccess.

The workflow component already makes the marking store configurable. So just build your own implementation using reflection for your project. Then, the fact that encapsulation gets broken happens only in your project (and is visible in it).
Another option is to just accept the fact that the marking store is outside the object and so needs a public API...

@fmata

This comment has been minimized.

Show comment
Hide comment
@fmata

fmata Aug 28, 2017

Contributor

Not only for me because my first needs is in symfony/workflow but again I understand your point and I agree. I ask here as RFC to have many opinions.

Maybe I can implement a logic in the marking store by myself... I'll see :-)

Thank you

Contributor

fmata commented Aug 28, 2017

Not only for me because my first needs is in symfony/workflow but again I understand your point and I agree. I ask here as RFC to have many opinions.

Maybe I can implement a logic in the marking store by myself... I'll see :-)

Thank you

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Aug 29, 2017

Member

Closing as explained.

Member

chalasr commented Aug 29, 2017

Closing as explained.

@chalasr chalasr closed this Aug 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment