[PropertyAccess] Added isReadable() and isWritable() #10570
Conversation
| PropertyAccess | ||
| -------------- | ||
|
|
||
| * The methods `isReadable()` and `isWritable()` were added to |
There was a problem hiding this comment.
hmm, sorry, no. This file documents only BC breaks, so not needed
There was a problem hiding this comment.
We never have this prefix in the UPGRADE file.
|
TODO before merging this, I'll see whether the |
| if (is_string($propertyPath)) { | ||
| $propertyPath = new PropertyPath($propertyPath); | ||
| } elseif (!$propertyPath instanceof PropertyPathInterface) { | ||
| throw new UnexpectedTypeException($propertyPath, 'string or Symfony\Component\PropertyAccess\PropertyPathInterface'); |
There was a problem hiding this comment.
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 for InvalidPropertyPathException anyway which can be raised by new PropertyPath($propertyPath).
There was a problem hiding this comment.
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 only an adder was present, but no remover (or vice versa), was removed.
|
I removed the |
|
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. |
| * @throws Exception\UnexpectedTypeException If a value within the path is neither object | ||
| * nor array | ||
| * @throws Exception\InvalidArgumentException If the property path is invalid | ||
| * @throws Exception\NoSuchPropertyException If a property does not exist or is not public. |
There was a problem hiding this comment.
phpdocs of these methods are missing NoSuchIndexException. I would simply document the parent class of both: AccessException
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. |
|
Huh, why not document it? |
|
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. |
|
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#3729 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. Commits ------- f7fb855 [PropertyAccess] Added missing exceptions to phpdoc 9aee2ad [PropertyAccess] Removed the argument $value from isWritable() 4262707 [PropertyAccess] Fixed CS and added missing documentation 6d2af21 [PropertyAccess] Added isReadable() and isWritable() 20e6bf8 [PropertyAccess] Refactored PropertyAccessorCollectionTest 0488389 [PropertyAccess] Refactored PropertyAccessorTest
| */ | ||
| private function findAdderAndRemover(\ReflectionClass $reflClass, array $singulars) | ||
| { | ||
| $exception = null; |
…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 | #3729 Hi 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 item 319bf29 [#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.