Skip to content

fix bug #74478 null coalescing operator failing with SplFixedArray - #2489

Closed
jhdxr wants to merge 2 commits into
php:PHP-7.0from
jhdxr:bugfix/74478
Closed

jhdxr wants to merge 2 commits into
php:PHP-7.0from
jhdxr:bugfix/74478

Conversation

@jhdxr

@jhdxr jhdxr commented Apr 20, 2017

Copy link
Copy Markdown
Member

fix bug #74478 null coalescing operator failing with SplFixedArray

Comment thread ext/spl/spl_fixedarray.c Outdated

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed. thank you.

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Member

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?)

@jhdxr jhdxr Apr 26, 2017 •

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member Author

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.

@keradus

keradus commented Apr 26, 2017

Copy link
Copy Markdown

@jhdxr thanks for picking up my bug report !

@jhdxr

jhdxr commented Apr 27, 2017

Copy link
Copy Markdown
Member Author

@keradus my pleasure.

ping @krakjoe .

@nikic

nikic commented May 24, 2017

Copy link
Copy Markdown
Member

Sorry for the delay, now merged as 872e43d into 7.0+. Thanks!

@nikic nikic closed this May 24, 2017
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.

4 participants