[PropertyAccess] Fix Regression in PropertyAccessor::isWritable() #42672
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
|
Hey! I think @HeahDude has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
| @@ -314,7 +314,7 @@ public function isWritable($objectOrArray, $propertyPath) | |||
| return false; | |||
| } | |||
| } else { | |||
| if (!$this->isPropertyWritable($zval[self::VALUE], $propertyPath->getElement($i))) { | |||
| if (!(\is_object($zval[self::VALUE]) && $this->isPropertyWritable($zval[self::VALUE], $propertyPath->getElement($i)))) { | |||
There was a problem hiding this comment.
I suggest using
!\is_object($zval[self::VALUE]) || !$this->isPropertyWritable($zval[self::VALUE], $propertyPath->getElement($i))that would make the extra parenthesis useless (helping readability IMHO), and prevent evaluating both conditions when the first is false (saving a method call).
There was a problem hiding this comment.
Are you sure about this change? if the value is not an object, then we should return true isn't it?
There was a problem hiding this comment.
Indeed you're right, I should have read the code below. Then the whole could be simplified as:
if ($propertyPath->isIndex($i)) {
return $zval[self::VALUE] instanceof \ArrayAccess || \is_array($zval[self::VALUE]);
}
return !\is_object($zval[self::VALUE]) || $this->isPropertyWritable($zval[self::VALUE], $propertyPath->getElement($i));There was a problem hiding this comment.
@HeahDude I've implemented your first suggestion due to improved readability.
In both cases the second mthod call was ommitted when the first failed. (false && xyz()) never calls xzy().
For the second suggestion. I've tried it, but numerous tests start to fail. I think it has something to do with turning the for loop into one that doesn't actually loop.
@jderusse The behaviour prior to the referenced commit was to return false when an array got passed with a propertyPath which wasn't an index. Then isPropertyWritable would have returned false which eventually made isWritable return false.
As of the mentioned commit the type definition for the first parameter changed from a DocBlock to the method itself. Php now evaluates the type during runtime and therefore throwing an exception.
Scope of this PR is to restore the bahaviour from symfony 5.2.
|
@HeahDude Did I forget something to add to this PR for it to get merged? Do I need to request someone to review this? Currently this bug blocks me from upgrading to 5.3. |
|
Thank you @haase-fabian. |
Since commit 6f6c1a16 a regression exists where calling
$propertyAccessor->isWritable()threw anTypeErrorwhen passing an array while using dot notation.$propertyAccessor->isWritable(["name" => "Bernard"], "name");