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

modified ObjectWalker to handle private properties in parent classes #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lgsgbr
Copy link

@lgsgbr lgsgbr commented Oct 12, 2018

ObjectWalker can handle private properties in parent classes with this modification. Tested. Please merge it into master.

@rdohms
Copy link
Owner

rdohms commented Nov 5, 2018

@lgsgbr does this have any side effects, other then it will start reading parent properties it used to not read?

I think it would be better to put this under a configuration flag, with the default not including parent properties to not break current usage and easily allow people to start using it with the flag.

@lgsgbr
Copy link
Author

lgsgbr commented Nov 14, 2018

@rdohms , no, it doesn't have any side effects. But I think, this is not a special case. Here is an example:

class A
{
    /**
     * @Filter\Trim
     */
    private $field;

    public function getField()
    {
       return $this->field;
    }
}

class B extends A
{
    /**
     * @Filter\Trim
     */
    private $anotherField;

    public function getAnotherField()
    {
        return $this->anotherField;
    }
}

$filter->filter(new A()); // it's OK
$filter->filter(new B()); // throws Exception

Without modificating the loader this example throws exception on class B because of a limitation in ReflectionClass::getProperties() method. Because of this I think it would be better to handle private properties in parent classes by default.
If you still prefer a configurable behavior, I can modify the solution. Shall I make it?

@rdohms
Copy link
Owner

rdohms commented Nov 19, 2018

@lgsgbr yes, if you could do that it would be great, less breakage and in the future we can then deprecate it.

Thank you very much!

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