[PropertyAccessor] Throw exception on nonexistant "index" on read access

[sescandell]

Currently, in the PropertyAccessor, if we try to get a nonexistent property from an array, PropertyAccessor will success and return a null value.
IMO, this is not logical compared to object behavior. In an object context, if property doesn't exist, it throws a NoSuchPropertyException exception.

I've made a patch for that taking into account read or write context for array.

• In write access, if property_path doesn't exist (for an array), there is no Exception (no BC break, no change).
• In a read access, if property_path doesn't exist (for an array), it throws a NoSuchIndexException exception (extending RuntimeException like NoSuchPropertyException).

Before making a PR, I would like to discuss it.

• What's your opinion? Do you think it is normal that PropertyAccessor doesn't throw exception in "array context" for read access?
• Should I throw a NoSuchPropertyException instead of NoSuchIndexException (is there a real importance to make a difference between Property and Index for exception... we are in a PropertyAccessor... user doesn't really care if it is a Index or not... no? ).
• I also think this PR introduces a BC (tests don't succeed anymore because of Tests\PropertyAccessorTest::testGetValueReadsArrayWithMissingIndexForCustomPropertyPath (wich is normal). In my patch I fixed it.

Commit concerned available here: sescandell@2e35cd7

Waiting for you comments.

[wouterj]

I think we should throw an exception if an index does not exist. But we maybe need to put this also in a builder, to prevent BC breaks.

For the exception, I think it should be NoSuchIndexException, but it should extend NoSuchPropertyException (the same as is done with the BadFunctionCallException and BadMethodCallException in php). That way, we can still typehint on the property exception.

[webmozart]

I think that your proposal is very good. In particular:

• In write access, if property_path doesn't exist (for an array), there is no Exception (no BC break, no change).
• In a read access, if property_path doesn't exist (for an array), it throws a NoSuchIndexException exception (extending RuntimeException like NoSuchPropertyException).

👍

I think you should create a common super-class for the NoSuch*Exceptions, for example AccessException. Then it is possible to either catch both exceptions in one clause or to catch them in two separate clauses.

[sescandell]

I'll do it and see what other people think about that.

[sescandell]

Hi again,

I've made a new commit. You can see changes here : https://github.com/sescandell/symfony/commits/patch-property_accessor_exception

@wouterj > I taked into account what you said and this feature is available throught the PropertyAccessorBuilder (already existing in the component). Thanks to that, there is no more BC Break.

@bschussek > In fact, NoSuch*Exception both extends RuntimeException... Do you think we really need to create an intermediate common class?

[wouterj]

thank you! Could you please create a PR, that makes it easier to disuss and review the patch.

And I agree with @bschussek, we should have a common class.

[stof]

@sescandell RuntimeException can be used for something else than AccessException. So catching all RuntimeException would not be equivalent to catching both NoSuch*Exception. This is why we would need the intermediate AccessException