Util->get() with Query throws exception if Query has no result

[aTanCS]

Util->get() with Query should return empty array if Query has no result instead of throwing exception 'Error getting property'. Or at least exception should be more specific (eg new exception NotFoundException), so it can be caught.

[boenrobot]

An empty array is definitely not the way to go. From experience, I know that people tend to not develop in a way where they don't check their output for errors, and then just wonder why they're not seeing the expected result at the router when "PHP seems to be running fine", so an exception is better for error handing.

As for a more specific exception... I think the RouterErrorException is specific enough. The problem is the router returned one or more errors. Anything beyond that is asking of the library to interpret these errors, but this is very fragile, as MikroTik may change the messages between RouterOS versions, and it will be a mess to reliably differenciate all.

In fairness, this particular error could be detected earlier (since there's a separate "print" call under the hood), and thus a separate exception will be reliable... But there's a different problem then, and that is that you now have two exceptions - one if the item is not found because the query returned nothing, and another if the router returned an error that says (in current RouterOS versions) "no such item". If you want to handle not found item errors in a universal fashion, you'd need to make two error handlers instead of one.

As it stands now (and how I think it should stay), you can detect that an error is coming from a get() call by inspecting not the message, but the code, e.g.

try {
    $item = $util->get(RouterOS\Query::where('name', 'an item'));
    $util->edit('another item', 'address', $item['comment']);
} catch (RouterOS\RouterErrorException $e) {
    if ($e->getCode() === RouterOS\RouterErrorException::CODE_GET_ERROR) {
        //Error is coming from the get() call
    } else {
        //Error is coming from somewhere else, in this case probably the edit()
    }
}

Now, the error codes are already a bitmask, so I guess I could add a more specific code for the not found due to a query error... If one wants to handle all types of get errors the same way, they could check the bitmask for a match, e.g.

try {
    $item = $util->get(RouterOS\Query::where('name', 'an item'));
    $util->edit('another item', 'address', $item['comment']);
} catch (RouterOS\RouterErrorException $e) {
    if (($e->getCode() & RouterOS\RouterErrorException::CODE_GET_ERROR)
        === RouterOS\RouterErrorException::CODE_GET_ERROR 
    ) {
        //Error is coming from the get() call
    } else {
        //Error is coming from somewhere else, in this case probably the edit()
    }
}

[aTanCS]

An empty array is definitely not the way to go.

getAll() already reaturns an empty array when there is nothing to return

As for a more specific exception..

Just to clarify, I need to know what happened with get() not if it was get(). So more specific error code or exception for get()'s Not found is what I need. But empty array like in getAll() would be enough, at least I don't have to handle them differently.

[boenrobot]

getAll() already reaturns an empty array when there is nothing to return

getAll() always returns a ResponseCollection. That collection may be empty if there are no items in the menu or items matching the criteria. In other words, an empty collection is an expected output in a normal operation, rather than a "special" output in an error situation. If errors were returning NULL for example, that would be a case of a special output.

Just to clarify, I need to know what happened with get() not if it was get(). So more specific error code or exception for get()'s Not found is what I need.

You can do that by using the getResponses() method of the exception. That gives you the exact error replies returned by RouterOS, and from there, you can get the exact message, code and whatever else RouterOS may decide to return in the future.

e.g.

try {
    $item = $util->get(RouterOS\Query::where('name', 'an item'));
    $util->edit('another item', 'address', $item['comment']);
} catch (RouterOS\RouterErrorException $e) {
    if ($e->getCode() === RouterOS\RouterErrorException::CODE_GET_ERROR) {
        //Error is coming from the get() call
        echo 'get command failed with the following (first) error from the router: ' .
            $e->getResponses()->getProperty('message');
    } else {
        //Error is coming from somewhere else, in this case probably the edit()
    }
}

There may be multiple errors from a single command, which is why a collection is returned, rather than a simple message.

[aTanCS]

I think we just don't understand each other. I will describe the problem again. Net_RouterOS evaluates it as error, because it causes RouterOS error. This should never happen, it should not send a api request if there no items to get.
if Query has an empty result, get() should return e.g. an empty array (as it is allowed in PHPDoc).

Part of your example:

$item = $util->get(RouterOS\Query::where('name', 'an item'));

Here is a commented content of get():

    /**
     * @return string|resource|null|array The value of the specified
     *     property as a string or as new PHP temp stream if the underlying
     *     {@link Client::isStreamingResponses()} is set to TRUE.
     *     If the property is not set, NULL will be returned.
     *     If $valueName is NULL, returns all properties as an array, where
     *     the result is parsed with {@link Script::parseValueToArray()}.
     */
public function get($number, $valueName = null)
    {
       // $number is instance of Query
        if ($number instanceof Query) {
            $number = explode(',', $this->find($number));
            // $this->find() did't find anything so it returned ','
            // after explode $number === ['']
            $number = $number[0];
            // $number === ''
            // here should be something like this, see below why
            // if ($number === '') return [];
        } elseif (is_int($number) || ((string)$number === (string)(int)$number)) {
            $this->find();
            if (isset($this->idCache[(int)$number])) {
                $number = $this->idCache[(int)$number];
            } else {
                throw new RouterErrorException(
                    'Unable to resolve number from ID cache (no such item maybe)',
                    RouterErrorException::CODE_CACHE_ERROR
                );
            }
        }

        $request = new Request($this->menu . '/get');
        // replaced $number with the value: 
        // $request->setArgument('number', ''); <= this is a wrong value for 'number',
        $request->setArgument('number', $number);
        $request->setArgument('value-name', $valueName);
        $responses = $this->client->sendSync($request);
        // RouterOS will return an error 'no such item' ($responses->getType() === '!trap')
        if (Response::TYPE_ERROR === $responses->getType()) {
            throw new RouterErrorException(
                'Error getting property',
                RouterErrorException::CODE_GET_ERROR,
                null,
                $responses
            );
        }

        $result = $responses->getProperty('ret');
        if (Stream::isStream($result)) {
            $result = stream_get_contents($result);
        }
        if (null === $valueName) {
            // @codeCoverageIgnoreStart
            //Some earlier RouterOS versions use "," instead of ";" as separator
            //Newer versions can't possibly enter this condition
            if (false === strpos($result, ';')
                && preg_match('/^([^=,]+\=[^=,]*)(?:\,(?1))+$/', $result)
            ) {
                $result = str_replace(',', ';', $result);
            }
            // @codeCoverageIgnoreEnd
            return Script::parseValueToArray('{' . $result . '}');
        }
        return $result;
    }

[boenrobot]

I understand, but I still don't think returning a value on failure is the right thing to do here...

I've now added b68b47e to the develop branch, which throws an exception on that event, with a new error code that is also matched by CODE_GET_ERROR.

So with that commit, you can do either

try {
    $item = $util->get(RouterOS\Query::where('name', 'an item'));
    $util->edit('another item', 'address', $item['comment']);
} catch (RouterOS\RouterErrorException $e) {
    if (($e->getCode() & RouterOS\RouterErrorException::CODE_GET_ERROR)
        === RouterOS\RouterErrorException::CODE_GET_ERROR 
    ) {
        //Error is coming from the get() call
    } else {
        //Error is coming from somewhere else, in this case probably the edit()
    }
}

If you want to handle all get errors in one place, or you can use

try {
    $item = $util->get(RouterOS\Query::where('name', 'an item'));
    $util->edit('another item', 'address', $item['comment']);
} catch (RouterOS\RouterErrorException $e) {
    if ($e->getCode() === RouterOS\RouterErrorException::CODE_GET_LOOKUP_ERROR) {
        //Error is coming from the get() call AND is about a query not having a match
    } else {
        //Error is coming either from the get() for another reason, or from somewhere else, in this case probably the edit()
    }
}

to handle the specific error.