Fix double access to ArrayAccess methods

[brunobg]

This is a proposal to fix #759

[coveralls]

Coverage increased (+0.004%) to 86.19% when pulling 62e807b on brunobg:patch-1 into 5289a54 on webonyx:master.

[mfn]

I'm supporting this change 👍

I discovered this some time ago and AFAIK suggested to lighthouse to adapt this but it wasn't picked up (and I also don't use lighthouse, I just noticed this in different Laravel / GraphQL adapter).

I tested this on a private project with lots of GraphQL tests, no issues found.

Are we concerned about a BC break here? Maybe we should considering target the next major version? #JustBeingCautious here

[spawnia]

@mfn great to see you around here.

This change indeed causes breakage when ArrayAccess::offsetGet() is implemented to throw when unset offsets are accessed. This behaviour is in line with how plain arrays work, I suspect many implementations in the wild work like this.

[brunobg]

@spawnia see my reply on the original issue and the new patch.

@mfn I think that should be as safe as the currently released code and doesn't even need a major version, could be part of a patch release. There are no changes to array and object.

There's a worst case scenario, with a trashy implementation of ArrayAccess. Let's say one that just wraps over a normal array, calling isset() on offsetExists() and accessing [] with offsetGet() with no checks, which generates a notice. But we shouldn't avoid this change because of that IMHO.

[brunobg]

Hey @spawnia anything else I can do about this? I'd love to get this accepted.

[spawnia]

The implementation looks fine, I would like to get more feedback. @vladar usually comes by every few weeks and merges a bunch of PRs.

[brunobg]

Considering other reports such as nuwave/lighthouse#1671, who knows how many people are having this behavior without noticing, please consider this PR and a new release ASAP @vladar and @spawnia . Thanks!

[spawnia]

This change does smooth over the weird semantics of ArrayAccess in a way that is beneficial to most if not all users.

[vladar]

This PR is trying to fix a problem in the wrong place. ArrayAccess is designed after arrays and it is quite expected to call isset (offsetExists) before trying to access an element. Take this simplest implementation of a class implementing ArrayAccess:

<?php
class Foo implements ArrayAccess
{
    private $foo = [];

    public function __construct(array $foo)
    {
        $this->foo = $foo;
    }

    public function offsetExists($offset)
    {
        return isset($this->foo[$offset]);
    }

    public function offsetGet($offset)
    {
        return $this->foo[$offset];
    }

    public function offsetSet($offset, $value)
    {
        $this->foo[$offset] = $value;
    }

    public function offsetUnset($offset)
    {
        unset($this->foo[$offset]);
    }
}

Now with the proposed change if you try to access a missing key like this:

$d = new Foo([ 'foo' => 'bar' ]);
$property = null;

try {
    $property = $d['bar'];
} catch (Throwable $e) {
    // pass
}

you will get PHP notice:

PHP Notice:  Undefined index: bar in Foo.php on line 18

Obviously, you can have an additional isset check in your offsetGet method but you are not required to! It is the responsibility of the ArrayAccess implementation to be smart enough not to do expensive operations multiple times.

So this is a breaking change and also an invalid behavior.

The generic implementation for defaultResolver should stay logically correct and consume interfaces like ArrayAccess in a way they were designed. You can always override the default resolver for your specific situation by passing fieldResolver argument in GraphQL::executeQuery or passing fieldResolver in StandardServer options.

Or alternatively you can set it globally via GraphQL\Executor\Executor::setDefaultFieldResolver($myDefaultResolver)

[spawnia]

The semantics of ArrayAccess are not that weird after all, having checked how they work in https://3v4l.org/OLqvr.

I think this change can be valid if you know you are dealing with inefficient implementations, such as Laravel's. As @vladar noted, it is easily possible to overwrite the default resolver in such a case.

[brunobg]

Closing this though I disagree as explained in the issue. Also, note that a detailed analysis of what happens in Laravel laravel/framework#36026 shows that real implementations can throw in offsetExists(). So if you're worried that a trivial implementation can throw a notice in my PR, you should be worried that it can also throw exceptions.