-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Cast Parameters #4568
Cast Parameters #4568
Conversation
Cast paramenters to int
What's the reasoning behind this change? |
Imho this change is not needed. The problem is in your userland code. |
In Issue #4563 @ptlis reported that if you pass parameters this way: $table = new TableGateway('some_table', $this->adapter);
$select = $table->getSql()->select();
$select->limit('25');
$select->offset('1');
$table->selectWith($select); // Exception is thrown here You get an Since the method is supposed to receive only int values i think nothing better than cast values to int if they are passed as string like in the example above avoiding any problems. Example: the parameters are being passed through a form where user may select the number of results to display, the values should be validated using Zend\Validator\Between (for example) but never converted to int and passed as method paramenters. I runned the tests and everything woredk fine, so i think is not a BC Breaker |
I clearly understand your problem. However I am against this change, as it adds additional work on every method call if done through the framework. Nothing is wrong with the code, as long as you pass correct values, as described in phpdoc. |
Not me, Butr after see the issue i take a look in the code and notice that in fact the framework wasn't really treating the parameters. My point is that if there is an interference in performance it will be minimum, but i'm really not a performace expert so i let you to decide to merge or not this change. |
I am not the decider here ;-) This is an open source project, i am just a contributor, just as you are. |
Yeah sure. I mean you in plural, all of you folks ;-) |
I think casting to int is a bad way to do this because this has unexpected side effects. For instance a string like Personally I think #4563 is not an issue, because if you provide correct parameters there is no problem. What can be discussed is the source of the problem of #4563 and that is if a number should be qouted at all, because there is no need to quote values only containing numbers. With numbers I only mean 0-9 and not numeric values which can also include for instance hexadecimal values. |
@prolic That's simply a testcase, the problem as I encountered it was with a value read from a config file. The response that "The problem is in your userland code" is thoroughly unreasonable IMHO as this behaviour is not idiomatic for PHP and additionally the query generated by the TableGateway::selectWith method differs from the query generated by Sql::getSqlStringForSqlObject (which generates a correct query). Surely any difference in query generation from the same objects should be considered to be a bug - one of these two methods should be changed to bring their behaviour inline with the other. The suggestion to add a conditional and call to |
And... What should i do?
|
I think idiomatic PHP would be leaving I think #2775 is relevant here, as I think this issue should be solved at #2775's level of abstraction if at all possible. |
@prolic PHP is quite intelligent when it comes to casting values to a type which they are already in, so this should not really be a large performance impact. I'd argue that this change is fine, as we internally fail when it is not an integer. Also I don't think that we should do a ctype_digit check, as that would really have a performance impact. |
Can you explain how you can make it internally fail when it is not an integer if you cast it to integer? I think this PR is not relevant anymore because the problem is fixed as @Gwis mentioned. Personally I think that fix is a better way to do it, because it targets the source of the problem rather than the consequences. |
I also think that the cast is not needed and it's dangerous, as @Martin-P suggested. Anyway it seems that this issue has been addressed by #2775. @ralphschindler what do you think? |
Actually, I ran into a similar problem just recently and I think I fixed it in my local branch by patching the Mysql Select decorator, but I think this is a better solution. Limit and offset should always be integers, there is no reason why we shouldn't cast them to such when the numbers are given as strings (which is the case when the offset and limit come from the web). The question is, should it be on set or should it be in the process functions? I am ok with something like this being on set as it is effectively doing lightweight parameter validation. Should we at least ensure is_numeric()? so that false/null doesn't get cast to 0? |
How would you handle a situation like my example with the string
The function is_numeric() also accepts hexadecimal values, binary values, octal values and numbers with an exponential part. I thought SQL injection was possible with hexadecimal values? |
Personally, I think casting to 2 is fine. If the user wanted any additional kind of validation of that particular parameter, then they can use Zend\Form or Zend\InputFilter to validate user input better.
My goal was to say "does this look number-ish", if so, cast it to an int. When it comes down to it though- I think even if you pass in null or false, perhaps that should be treated as a 0 for consistency. Along the same argument as above, if you need a buffer/validator between user input and Zend\Db, you can incorporate that kind of filtering validation into your app. |
I think I've found a compromise here. I will add an is_numeric() check in limit() and offset(). If it's not, it will throw an exception, if it is, then in processLimit/processOffset the numeric will be cast to integer. This allows getRawState() to return the actual values that were provided in the original offset()/limit() calls. If you give the call something that is number-ish, I think it's ok to allow numberish (is_numeric) stuff to be cast to integers for database usage. @pauloelr do you want me to handle this or could you? If you, what kind of availability do you have? |
Hi @ralphschindler, i can handle it yet today, just fiishing some work tasks here and i'll do that in sequence |
Cating values on processLimit() and processOffset()
Hi @ralphschindler i did the changes but i'm not in my main machine and i'm unable to run the tests right now, but since is a small change i don't think there will be any problems, but if anyone could run the tests i'll be thankful. |
Personally, I would make the exception message more informative than just throw new Exception\InvalidArgumentException(sprintf(
'%s expects parameter to be numeric, "%s" given',
__METHOD__,
(is_object($limit) ? get_class($limit) : gettype($limit))
)); Edit: Same for offset ofcourse |
Travis is still returning Code Standards error but in a Zend/Crypt/Password/Bcrypt.php and i didn't change this file. |
@pauloelr The CS issue is not yours; don't worry about it at this time. (I've fixed it.) |
Awesome, looks pretty good, I'll add a unit tests and merge, thanks @pauloelr ! |
__METHOD__, | ||
(is_object($limit) ? get_class($limit) : gettype($limit)) | ||
)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IF we merge this, simplify the above:
if (!is_numeric($limit)) {
// throw the exception
}
$this->limit = $limit;
This is more consistent with the rest of the framework, and makes it more obvious what you're checking, and what constitutes an error condition.
Same remark applies elsewhere.
Just tested this with pdo_mysql; this patch does not seem to fix the issue with the limit if for instance a string "25" is passed in. |
I know about this and will make similar changes (as we discussed @mwillbanks) to the MySqlPlatform Sql class. |
Merge branch 'pauloelr-zend_db_select_cast_limit_and_offset'
Merge branch 'pauloelr-zend_db_select_cast_limit_and_offset' into develop
Cast paramenters to int Issue #4563