-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[2.4][PropertyAccess] Fixed getValue() when accessing non-existing indices of ArrayAccess implementations #10947
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
Conversation
@@ -428,6 +444,8 @@ private function findAdderAndRemover(\ReflectionClass $reflClass, array $singula | |||
)); | |||
} | |||
} | |||
|
|||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be reverted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree. Methods with a return value should always return a value, even if it is null
. Relying on the implicit return value is confusing when reading the code (was the return statement forgotten?) - which was exactly what happened to me now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@webmozart I see you missed one of 'strangest' PRs in Symfony =) #10717.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you revert this change please. This changes has been done everywhere now in all versions of Symfony, and we are not going to revert this. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@webmozart To be honest, I also prefer the explicit return, and I keep using it in my own projects. However, the code of Symfony should follow the rules stated for Symfony.
…existing indices of ArrayAccess implementations (webmozart) This PR was merged into the 2.4 branch. Discussion ---------- [2.4][PropertyAccess] Fixed getValue() when accessing non-existing indices of ArrayAccess implementations | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This PR backports two test refactoring commits from the master branch. The third commit is basically the same as #10946. Commits ------- 5b5a6b6 [PropertyAccess] Fixed getValue() when accessing non-existing indices of ArrayAccess implementations 91ee821 [PropertyAccess] Refactored PropertyAccessorCollectionTest 5614303 [PropertyAccess] Refactored PropertyAccessorTest
@@ -42,7 +42,7 @@ class PropertyAccessor implements PropertyAccessorInterface | |||
public function __construct($magicCall = false, $throwExceptionOnInvalidIndex = false) | |||
{ | |||
$this->magicCall = $magicCall; | |||
$this->throwExceptionOnInvalidIndex = $throwExceptionOnInvalidIndex; | |||
$this->ignoreInvalidIndices = $throwExceptionOnInvalidIndex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be $this->ignoreInvalidIndices = !$throwExceptionOnInvalidIndex;
, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah of course, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR backports two test refactoring commits from the master branch. The third commit is basically the same as #10946.