-
Notifications
You must be signed in to change notification settings - Fork 7.9k
fix bug #74478 null coalescing operator failing with SplFixedArray #2489
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
Conversation
ext/spl/spl_fixedarray.c
Outdated
@@ -358,6 +358,20 @@ static zval *spl_fixedarray_object_read_dimension(zval *object, zval *offset, in | |||
|
|||
intern = Z_SPLFIXEDARRAY_P(object); | |||
|
|||
if (type == BP_VAR_IS) { |
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 should only be done if the method has been overridden.
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.
Though you might want to simply delegate to spl_fixedarray_object_has_dimension here.
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.
fixed. thank you.
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.
I don't choose to call spl_fixedarray_object_has_dimension
because I think the call to spl_fixedarray_object_has_dimension_helper
inside that function is not needed here.
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.
Might it not be necessary in the (admittedly rather odd) case where offsetGet has been overridden but offsetHas not? In that case we should still perform the isset based on default semantics (or not?)
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.
Good point. IMHO we should follow current logic, simply call offsetGet
, if offsetGet
is overridden while offsetHas
not.
The reason why bug #74478 is a bug is $data['foo'] ?? 42;
equals to isset($data['foo']) ? $data['foo'] : 42;
. We override offsetHas
because we hope we can define how isset()
works, and this bug ruled out the ability. If we don't override offsetHas
, it indicates I'm pretty satisfied with how isset()
works now, so we just need to follow current logic.
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.
So what I had in mind here is something like this:
<?php
class MyArray extends SplFixedArray {
public function offsetGet($key) {
return "prefix_" . parent::offsetGet($key);
}
}
$arr = new MyArray(1);
var_dump(isset($arr[0]) ? $arr[0] : 42); // Returns 42
var_dump($arr[0] ?? 42); // According to your patch (I think) this will throw rather than returning 42
Due to a typo only extending offsetGet didn't actually work, just fixed this in 1967950.
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.
yes. In fact I have fixed this in my patch as well.
@jhdxr thanks for picking up my bug report ! |
Sorry for the delay, now merged as 872e43d into 7.0+. Thanks! |
fix bug #74478 null coalescing operator failing with SplFixedArray