Skip to content
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

Fix return types & end host on slash #93

Closed
repat opened this issue Sep 16, 2020 · 3 comments
Closed

Fix return types & end host on slash #93

repat opened this issue Sep 16, 2020 · 3 comments

Comments

@repat
Copy link
Contributor

repat commented Sep 16, 2020

Code

This is a sample all() method, e.g. for blocks. The PHP return type is only array

    public function all(array $query = []): array
    {
        return $this->get('blocks', $query);
    }

-- src/API/Blocks.php

However, it's using the get() method from AbstractAPI, which doesn't have a return type. The PHPDoc suggests array or string.

    /**
     * Send a GET request with query parameters.
     *
     * @param string $path
     * @param array  $query
     *
     * @return array|string
     */
    protected function get(string $path, array $query = [])
    {
        $response = $this->connection->getHttpClient()->get($path, compact('query'));

        return json_decode($response->getBody()->getContents(), true);
    }

-- src/API/AbstractAPI.php

However, json_decode can never return a string, only bool, array and null (and technically int, when first argument is true).

>>> json_decode('', true); // e.g. empty API response
=> null

Expected Behavior

I would expect that I either received an empty array or null. I would prefer null as this symbolizes the API response was empty, vs. an empty array could just be the actual response with valid JSON ({} / []).

Current Behavior

Screenshot

image

We can't get around this e.g. with try/catch as it's return types are a PHP language feature.

Possible Solution

Simplest solution imho: Add ? to return type so it can accept null coming from json_decode.

    public function all(array $query = []): ?array
    {
        return $this->get('blocks', $query);
    }

-- src/API/Blocks.php

Steps to Reproduce (for bugs)

Use a host URL not ending on a slash (/). If you're add a slash at the end the connection works.

$connection = new Connection([
                'host' => 'https://explorer.ark.io/api' // NOTE no trailing slash
            ]);
$connection->blocks()->all(); // will now cause error

// ---

$connection = new Connection([
                'host' => 'https://explorer.ark.io/api/' // NOTE this one has a trailing slash
            ]);
$connection->blocks()->all(); // works

Your Environment

PHP: 7.4.10
php-client: 0.1.3
Laravel: 8.3
MacOS 10.14.6

@ghost
Copy link

ghost commented Sep 16, 2020

Thanks for opening this issue! A maintainer will review this in the next few days and explicitly select labels so you know what's going on.

If no reviewer appears after a week, a reminder will be sent out.

This was referenced Sep 16, 2020
@stale
Copy link

stale bot commented Oct 16, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: Stale The pull request is in need of updates but there has not been a sufficient response. label Oct 16, 2020
@stale stale bot closed this as completed Oct 24, 2020
@ghost
Copy link

ghost commented Oct 24, 2020

This issue has been closed. If you wish to re-open it please provide additional information.

@ghost ghost removed the Status: Stale The pull request is in need of updates but there has not been a sufficient response. label Oct 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant