Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[PropertyAccess] Added isReadable() and isWritable()#10570
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
missing[BC Break] prefix
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.
hmm, sorry, no. This file documents only BC breaks, so not needed
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.
We never have this prefix in the UPGRADE file.
webmozart commentedMar 29, 2014
TODO before merging this, I'll see whether the |
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.
Imho UnexpectedTypeException is not semantically correct because its defined as RuntimeException but here it is clearly a LogicException. UnexpectedTypeException is correctly used for checks whether value in path is array or object. But we should destringuish these two cases.
So for this case here, we should instead throw a InvalidPropertyPathException and it should extend LogicException instead.
Currently all methods in AccessorInterface are missing phpdoc forInvalidPropertyPathException anyway which can be raised bynew PropertyPath($propertyPath).
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.
Currently isReadable and isWriteable phpdoc are missing UnexpectedTypeException and InvalidPropertyPathException. With my suggestion above, only InvalidPropertyPathException could be raised.
To keep isWritable() and setValue() consistent, the exception thrown when onlyan adder was present, but no remover (or vice versa), was removed.
webmozart commentedMar 30, 2014
I removed the |
Tobion commentedMar 30, 2014
InvalidPropertyPathException should extend InvalidArgumentException (which is a tiny bc break). But otherwise we would need to add InvalidPropertyPathException too all phpdoc because InvalidPropertyPathException would not be convered since it extends a RuntimeException. |
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.
phpdocs of these methods are missing NoSuchIndexException. I would simply document the parent class of both: AccessException
webmozart commentedMar 31, 2014
Since strings can be constructed at runtime, it is indeed a RuntimeException. As such, I think it's fine not to document it. This should be ready to merge now. |
Tobion commentedMar 31, 2014
Huh, why not document it? |
webmozart commentedMar 31, 2014
You're right, but
is not true: The caller can't enforce that a property path (string) is correct (except by passing a PropertyPath instance), so this case is different than, for example, type errors, that can be prevented by using type checks or casting. |
Tobion commentedMar 31, 2014
One can see it both ways. But it still needs to be documented. |
…webmozart)This PR was merged into the 2.5-dev branch.Discussion----------[PropertyAccess] Added isReadable() and isWritable()| Q | A| ------------- | ---| Bug fix? | no| New feature? | yes| BC breaks? | yes| Deprecations? | no| Tests pass? | yes| Fixed tickets |#8659| License | MIT| Doc PR |symfony/symfony-docs#3729This PR introduces BC breaks that are described in detail in the UPGRADE file. The BC breaks conform to our policy. They shouldn't affect many people, so I think we can safely do them.Commits-------f7fb855 [PropertyAccess] Added missing exceptions to phpdoc9aee2ad [PropertyAccess] Removed the argument $value from isWritable()4262707 [PropertyAccess] Fixed CS and added missing documentation6d2af21 [PropertyAccess] Added isReadable() and isWritable()20e6bf8 [PropertyAccess] Refactored PropertyAccessorCollectionTest0488389 [PropertyAccess] Refactored PropertyAccessorTest
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 looks like a dead assignment
…dable() and isWritable() methods (webmozart)This PR was merged into the master branch.Discussion----------Added documentation for the new PropertyAccessor::isReadable() and isWritable() methods| Q | A| ------------- | ---| Doc fix? | no| New docs? | yes (symfony/symfony#10570)| Applies to | 2.5+| Fixed tickets | -Commits-------bb8e3ed Added documentation for the new PropertyAccessor::isReadable() and isWritable() methods
This PR was merged into the master branch.Discussion----------Property access tweaks| Q | A| ------------- | ---| Doc fix? | yes| New docs? | no| Applies to | 2.5+| Fixed tickets |#3729Hi guys!This follows after#3729 - it includes several small suggestions made by@wouterj and@xabbuh.Additionally, this removes the 3rd argument to `isWritable`, which was removed in the orignal PR (symfony/symfony#10570) and didn't make it into the final feature.Thanks!Commits-------fb9fe99 [#3729] Removing 3rd argument to isWritable - this doesn't exist in the final merged item319bf29 [#3729] Making minor changes thansk to@wouterj and@xabbuh.
This PR introduces BC breaks that are described in detail in the UPGRADE file. The BC breaks conform to our policy. They shouldn't affect many people, so I think we can safely do them.