10000 [PropertyAccess] setValue (again) replace array when not needed · Issue #18437 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[PropertyAccess] setValue (again) replace array when not needed #18437

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
MisatoTremor opened this issue Apr 4, 2016 · 3 comments
Closed

[PropertyAccess] setValue (again) replace array when not needed #18437

MisatoTremor opened this issue Apr 4, 2016 · 3 comments
Labels
Bug PropertyAccess Status: Needs Review

Comments

@MisatoTremor
Copy link
Contributor

This behavior has been mentioned in #13731 by @Tobion and fixed in #13835 by @bananer but was reintroduced later (my guess would be #18224 as it introduced the code that replaces the array).

In the following test case the second assertion fails because after the setValue call the publicAccessor property of object has the value array('value2' => 'baz') instead of array('value1' => 'foo', 'value2' => 'baz').

    public function testArrayNotBeeingOverwritten()
    {
        $value = array('value1' => 'foo', 'value2' => 'bar');
        $object = new TestClass($value);

        $this->propertyAccessor->setValue($object, 'publicAccessor[value2]', 'baz');
        $this->assertSame('baz', $this->propertyAccessor->getValue($object, 'publicAccessor[value2]'));
        $this->assertSame(array('value1' => 'foo', 'value2' => 'baz'), $object->getPublicAccessor());
    }
@nicolas-grekas
Copy link
Member

That's really likely! The fix should go on 2.3. Anyone willing to work on it?

@MisatoTremor
Copy link
Contributor Author

Update: Fixed the test case.

@MisatoTremor
Copy link
Contributor Author

@nicolas-grekas
Assigning $zval[self::REF] with the current value after src/Symfony/Component/PropertyAccess/PropertyAccessor.php, Line 153 like below fixes this, but seems a bit brute for the intention of your PR.
Could you please check if this negatively affects your benchmark results?

Before:

$ref = &$zval[self::REF];

After:

$ref = &$zval[self::REF];
$zval[self::REF] = $zval[self::VALUE];

@nicolas-grekas nicolas-grekas mentioned this issue Apr 5, 2016
Merged
fabpot added a commit that referenced this issue Apr 7, 2016
@fabpot
bug #18449 [PropertyAccess] Fix regression (nicolas-grekas)
416f7d7 
This PR was merged into the 2.3 branch.

Discussion
----------

[PropertyAccess] Fix regression

| Q             | A
| ------------- | ---
| Branch?       | 2.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? |
| Tests pass?   | yes
| Fixed tickets | #18437
| License       | MIT
| Doc PR        | -

All credits go to @MisatoTremor

I don't measure any perf impact.

Commits
-------

2b30d48 [PropertyAccess] Fix regression
@fabpot fabpot closed this as completed Apr 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug PropertyAccess Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fabpot @javiereguiluz @nicolas-grekas @MisatoTremor @carsonbot
0