Skip to content

[PropertyAccess] Bug: Throw exception on no such index #18970

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

Closed

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Jun 5, 2016

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? not yet
Fixed tickets #18969
License MIT
Doc PR n/a

Patch for #18969, if it is confirmed, I'll adapt the tests.
It seems that tests expect this behavior but I find that really strange.

@chalasr chalasr changed the title Throw exception on no such index [PropertyAccess] Bug: Throw exception on no such index Jun 5, 2016
@xabbuh
Copy link
Member

xabbuh commented Jun 5, 2016

As getValue() returns null throwing an exception in isReadable() seems inconsistent to me. Though changing getValue() would be a BC break.

@chalasr
Copy link
Member Author

chalasr commented Jun 6, 2016

The method that would throw an exception is readIndex() that is called for both isReadable and getValue, such as readProperty for an object that behaves as proposed here.
Maybe catch the exception in getValue to keep BC, WDYT? That would fix isReadable at least.

@chalasr
Copy link
Member Author

chalasr commented Jun 6, 2016

Actually, isReadable() always returns true for an array because readIndex() doesn't throw an exception if the index doesn't exist and isReadable() returns false only if an exception is thrown in readIndex()/readProperty().

So with this the return type would stay intact, just fixed as it is bugged.
In addition of throwing a NoSuchIndexException in readIndex() to return the good boolean, I propose to catch the NoSuchIndexException in getValue() if throwing it would break our BC promise (not sure that don't catch it would break it as the thrown exception should be already expected).

@chalasr chalasr closed this Jun 6, 2016
@chalasr chalasr deleted the bugfix_propertyaccess_isreadable_array branch June 6, 2016 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants