jakoch / PHPTracRPC

A PHP library to interact with the Trac Bugtracker API via remote procedure calls.
4 stars 2 forks source link

When getting a multicall response, getResponse returns current() on the array #10

Closed mpedrummer closed 8 years ago

mpedrummer commented 8 years ago

There's a check to see if $id === true, and I think it should be $id !== true, given that the line inside that check looks to see if $id is an array, which is impossible if $id === true.

jakoch commented 8 years ago

I agree that $id === true is not correct. I think it should be $id == true or better just isset($id), because we need to test if $id is set. $id might be an integer or an array of integers. That an $id array is also possible is also not mentioned in the doc block.

Cleanup incoming...

mpedrummer commented 8 years ago

Yeah, that makes sense, too. The company I work for is super-strict about avoiding loose type checks, so it didn't cross my mind at first!

Want me to change it, or are you working on it?

jakoch commented 8 years ago

I've rewritten the method. Could you take a look?

    /**
     * Get the response from the request.
     *
     * @param  mixed|int|array  The id of the call (or an array of ids).
     * @return object stdClass
     */
    public function getResponse($id = false)
    {
        // response is an object
        if (is_object($this->response)) {
            return $this->response;
        } 

        // response is an array
        if (is_array($this->response)) {

            // response is an array - but no id requested
            if($id === false) {
                return current($this->response);
            }

            // response is an array - with id requested
            if (isset($id) && is_int($id)) {
                return $this->response[$id];
            }

            // response is an array - with an array of id's requested
            if (isset($id) && is_array($id)) {
                $ret = array();
                foreach ($id as $key) {
                if (false === isset($this->response[$key])) {
                   continue;
                }
                $ret[$key] = $this->response[$key];
                return $ret;
            }
        }

        return false;
    }
mpedrummer commented 8 years ago

That looks great - and I cut and pasted it into my tests, works as expected.

mpedrummer commented 8 years ago

Hey, there's actually a curly brace mismatch in this - the foreach in the "response is an array" section isn't indented correctly. I thought I'd just missed the final brace when cutting and pasting, but after pulling down the latest, it's actually in there.

jakoch commented 8 years ago

damn, i'm so sloppy... fixed that one too

Closing. Hope everything works as expected.

Thank you for contributing and making this thingy better :+1: