The JSONPath constructor is unsafe

[guillemcanal]

🐛 Bug Report

Since we upgraded softcreatr/jsonpath to ^0.7.2, we noticed that some type checks were added to the JsonPath object (which is awesome 😍).

Here's the issue

class JsonPath
{
    // ...

    /**
     * @param array|ArrayAccess $data
     * @param bool $options
     */
    final public function __construct($data = [], bool $options = false)
    {
        // Here, we should assert that `$data` is either an `array` or an instance of `ArrayAccess`, if not an `InvalidArgumentException` should be thrown
        $this->data = $data;
        $this->options = $options;
    } 
    // ...
    public function getData(): array
    {
        // it will although returns a `TypeError` when `$data` is an instance of `ArrayAccess`
        return $this->data;
    }
    // ...
}

Have you spent some time to check if this issue has been raised before?

[x] I have read googled for a similar issue or checked our older issues for a similar bug

Have you read the Code of Conduct?

[x] I have read the Code of Conduct

To Reproduce

(new JsonPath(\json_decode($payload)))->getData();
// TypeError: JsonPath::getData(): Return value must be of type array, stdClass returned

Expected behavior

An InvalidArgumentException

Actual Behavior

No exception is thrown

Your Environment

$ cat /etc/*-release
3.13.1
NAME="Alpine Linux"
ID=alpine
VERSION_ID=3.13.1
PRETTY_NAME="Alpine Linux v3.13"
HOME_URL="https://alpinelinux.org/"
BUG_REPORT_URL="https://bugs.alpinelinux.org/"
$ php -v
PHP 8.0.2 (cli) (built: Feb  5 2021 04:31:24) ( NTS )
Copyright (c) The PHP Group
Zend Engine v4.0.2, Copyright (c) Zend Technologies
    with Xdebug v3.0.2, Copyright (c) 2002-2021, by Derick Rethans

Thank you, it you need additional information, feel free to contact me :) I can provide a "fix" if required

[SoftCreatR]

Hi,

thank you for your report. Checking the code, I'm somewhat unconfident, that the current implementation is correct. Without testing it, the problem I see with your proposed change (and the current implementation) is the fact, that you should be able to pass nearly everything to the constructor, even strings, but getData should return either an array or an object. The easiest fix might be to modify the return type for getData() to array|ArrayAccess. However, union types only exist since PHP 8, so the fix might be to replace

    public function getData(): array

with

    /**
     * @return array|ArrayAccess
     */
    public function getData()

[guillemcanal]

@SoftCreatR I see. I haven't checked the test suite yet, but removing the return type hint, seems to be a pretty good solution. Beware however, because it seems that \stdClass objects generated by json_decode don't implements ArrayAccess, type checkers such as PHPStan or Psalm will complain :)

[SoftCreatR]

Well, simply declare it as mixed 😄