getParam() method ignores "default" parameter #70

Closed
freshheads opened this Issue Jun 15, 2011 · 6 comments

Projects

None yet

4 participants

@ghost
ghost commented Jun 15, 2011

The getParam() method of the Frapi_Action class accepts a "default" parameter (The default value, if param is empty). Apparently, this parameter is never used.

I wanna do something similar like:

$limit = $this->getParam('limit', self::TYPE_INTEGER, 20);

When no or an invalid limit is passed, the function will return 0, this seems incorrect behavior.

It should return the default (20) when the param is:

  • Not passed
  • Empty (empty string, but does the number zero count as empty also?)
  • Invalid ('abc' casted to integer returns 0, but can it be interpreted as empty?)

The $error_name parameter (The error name to raise, as last resort) also seems unused. Additionally, it would be useful to have a simple hasParam() which returns a boolean.

If you need any help implementing, let me know!

Owner

This is very interesting, if I can reproduce, it is indeed a bug which I'll fix :) I won't have time to get to it before Friday so if you feel like taking a jab at it go ahead and let me know when you request a pull :)

Contributor

Just added a pull request which fixes the issue. What still isn't handled is "invalid" parameters. What should happen when the following requests are performed?

http://api/books?limit=abc10
Current behavior: getParam('limit', self::TYPE_INTEGER, 20) returns 0, because it is typecasted to an integer. But shouldn't it return null/false, fallback to the default or throw an exception?

http://api/books?limit=10abc
Current behavior: getParam('limit', self::TYPE_INTEGER, 20) returns 10, because it is typecasted to an integer. But shouldn't it return null/false, fallback to the default or throw an exception?

http://api/books?limit=abc
Current behavior: getParam('limit', self::TYPE_INTEGER, 20) returns 0, because it is typecasted to an integer. But shouldn't it return null, false, the default value, or maybe even throw an exception?

http://api/books?limit=
Current behavior: getParam('limit', self::TYPE_INTEGER, 20) returns 0, because an empty string is typecasted to an integer. But shouldn't it return null, false, the default value, or maybe even throw an exception?

What do you think, David?

Owner

It should essentially do:

if (is_numeric($param)) { 
    return $param; 
} 
return $default;

I believe this will fix a most of those issues.

Contributor

The risk of that solution is that an API client could assume his parameters are valid, while it uses a default value instead. This could return an unexpected response to the client. I believe the client has to receive an error about an invalid parameter, but the big question is: where does this have to be handled? Is the getParam() method the right place for standard type validation or does it just typecast? David, could you explain something about how the typecasting parameter initially was meant?

Owner

You are right, let's jump on the IRC channel and talk with the lads. If you want to initiate the discussion via the frapi-dev mailing list that'd be nice as well :)

Owner
dshafik commented May 29, 2012

Fixed with #160

@dshafik dshafik closed this May 29, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment