8000 [Form] PropertyPath readValue fails for custom array objects by asm89 · Pull Request #4606 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] PropertyPath readValue fails for custom array objects #4606

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[Form] PropertyPath readValue fails for custom array objects #4606

wants to merge 2 commits into from

Conversation

asm89
Copy link
Contributor
@asm89 asm89 commented Jun 18, 2012

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: Build Status
License of the code: MIT

This PR addresses issue #4535. I added previously failing tests and a possible fix for the issue. It re-introduced a 'special-case' for real arrays. I'd like to have feedback from @bschussek.

Travis will possible say this PR fails after merge because current symfony-master is failing.

@travisbot
Copy link

This pull request fails (merged f7dd770 into 0b8b76b).

@uwej711
Copy link
Contributor
uwej711 commented Jun 18, 2012

There is a slightly more complicated case that still fails:

public function testSetCustomArrayCollection()
{
    $collection = new \Symfony\Component\Form\Tests\Fixtures\CustomArrayObject(array('foo', array('bar')));
    $path = new PropertyPath('[1][0]');
    $path->setValue($collection, 'baz');
    $this->assertEquals('baz', $collection[1][0]);
}

Looks like reading properties works without all the references. Setting is then the harder case: maybe walk down the path until you can set the property and setting the changed values the way up all the parent properties ... (in the example above go down until you have the array('bar'), set index at 0, take the modified array and set it at index 1 of the collection ...

@uwej711
Copy link
Contributor
uwej711 commented Jun 19, 2012

See #4612

@asm89
Copy link
Contributor Author
asm89 commented Jun 19, 2012

Cool! You could have worked from my branch. ;) But it seems that you fixed it? I'll close this PR in favor of yours. #4612

@asm89 asm89 closed this Jun 19, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@asm89 @travisbot @uwej711
0