add return type to Uploader

[tacman]

Okay to add the return type?

    public function uploader()

    /**
     * Get an uploader with which to upload photos to (or replace photos on) Flickr.
     * @return Uploader
     */
    public function uploader(): Uploader // add the return type
    {
        return new Uploader($this);
    }

[samwilson]

Looks like that'd be fine? There's a bunch of places we could delete the @return and instead have return type hints.

[tacman]

yes. rector can likely make short work of this!

BTW, the version is wrong here: https://github.com/samwilson/phpflickr/blob/6.0.0/src/PhpFlickr.php

[samwilson]

Oh oops! Fixing in #82.

[tacman]

I used php-cs-fixer to add return types, but tests are failing now because of those return types. Since 6.0 was just released, what do you think about tweaking the return types so that instead of returning false in certain situations, we return null.

For example, instead of returning array|bool, returning ?array. The original code had return false when the cache wasn't found.

Or maybe I'll just tweak the return types to include bool, but to me it seems odd to return an array or a bool.

    /**
     * Get a cached request.
     * @param string[] $request Array of request parameters ('api_sig' will be discarded).
     * @return string[]
     */
    public function getCached($request): ?array
    {
        //Checks for a cached result to the request.
        //If there is no cache result, it returns a value of null. If it finds one,
        //it returns the unparsed XML.
        unset($request['api_sig']);
        foreach ($request as $key => $value) {
            if (empty($value)) {
                unset($request[$key]);
            } else {
                $request[$key] = (string) $request[$key];
            }
        }
        $cacheKey = md5(serialize($request));

        if ($this->cachePool instanceof CacheItemPoolInterface) {
            $item = $this->cachePool->getItem($cacheKey);
            if ($item->isHit()) {
                return $item->get();
            } else {
                return null;
            }
        }
        return null;
    }

[tacman]

Actually, I just fixed the return type. Where do I set the API key so that the tests can run?

[samwilson]

The API key should be in tests/config.php (and examples/config.php for those).

I think returning null to indicate not cached is probably fine. We never want to cache a null.

[samwilson]

Or you can set them as the env vars FLICKR_API_KEY, FLICKR_API_SECRET, FLICKR_ACCESS_TOKEN, FLICKR_ACCESS_SECRET.

[tacman]

set them as the env vars FLICKR_API_KEY, FLICKR_API_SECRET, FLICKR_ACCESS_TOKEN, FLICKR_ACCESS_SECRET.

I think that's the better way, to avoid accidentally checking in secrets into github. But really these test calls need to be mocked, something I've read about but haven't done yet.