Skip to content

[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

Merged
merged 3 commits into from
May 21, 2014

Conversation

webmozart
Copy link
Contributor

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.

@@ -428,6 +444,8 @@ private function findAdderAndRemover(\ReflectionClass $reflClass, array $singula
));
}
}

return null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be reverted

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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.

@fabpot fabpot merged commit 5b5a6b6 into symfony:2.4 May 21, 2014
fabpot added a commit that referenced this pull request May 21, 2014
…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;
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah of course, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

4 participants