Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Cast Parameters #4568

Merged
merged 6 commits into from
Jun 11, 2013
Merged

Cast Parameters #4568

merged 6 commits into from
Jun 11, 2013

Conversation

pauloelr
Copy link
Contributor

Cast paramenters to int Issue #4563

Cast paramenters to int
@bayleedev
Copy link
Contributor

What's the reasoning behind this change?

@prolic
Copy link
Contributor

prolic commented May 30, 2013

Imho this change is not needed. The problem is in your userland code.

@pauloelr
Copy link
Contributor Author

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 InvalidQueryException

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

@prolic
Copy link
Contributor

prolic commented May 30, 2013

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.
Passing strings as limit param is a mistake done in your userland code. It's your responsibility to validate and filter all incoming parameters correctly, before you pass them to any method. I assume hereby that this was the reason for having limit('1') instead of limit(1). Or did you really hardcoded limit('1') in your code?

@pauloelr
Copy link
Contributor Author

Not me,
In fact i had never reach this problem before the reported issue #4563.

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.

@prolic
Copy link
Contributor

prolic commented May 30, 2013

I am not the decider here ;-) This is an open source project, i am just a contributor, just as you are.

@pauloelr
Copy link
Contributor Author

Yeah sure.

I mean you in plural, all of you folks ;-)

@Martin-P
Copy link
Contributor

I think casting to int is a bad way to do this because this has unexpected side effects. For instance a string like 2 hamburgers is suddenly completely correct as it is being cast to 2. I hope you agree that should not be possible. A more solid way to do this, is to at least check with ctype_digit() if it is a correct value before casting it to int.

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.

@ptlis
Copy link

ptlis commented May 31, 2013

@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 ctype_digit by @Martin-P is how I would approach the problem. Discussion of the performance effects of a check like this just smacks of foolish micro-optimisation without any evidence to justify the requirements for it - if you're using a query builder the performance difference for one additional conditional is going to be a tiny portion of the execution time.

@pauloelr
Copy link
Contributor Author

pauloelr commented Jun 2, 2013

And... What should i do?

  1. Verify if there is only digits with ctype_digit() and throw an ImvalidArgumentException if is not?
  2. Leave this way to merge?
  3. Close the PR to let the things the way it was before?

@gws
Copy link
Contributor

gws commented Jun 2, 2013

I think idiomatic PHP would be leaving Zend\Db\Sql\Select alone. PHP happily performs type juggling, so conceptually we don't have a problem until we convert our PHP object to another language with different rules. Therefore, I think the right place to address this is at the point of platform-specific SQL conversion.

I think #2775 is relevant here, as I think this issue should be solved at #2775's level of abstraction if at all possible.

@DASPRiD
Copy link
Member

DASPRiD commented Jun 6, 2013

@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.

@Martin-P
Copy link
Contributor

Martin-P commented Jun 6, 2013

I'd argue that this change is fine, as we internally fail when it is not an integer.

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.

@ezimuel
Copy link
Contributor

ezimuel commented Jun 6, 2013

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?

@ralphschindler
Copy link
Member

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?

@Martin-P
Copy link
Contributor

Martin-P commented Jun 6, 2013

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).

How would you handle a situation like my example with the string 2 hamburgers? In your opinion that should be a correct value for limit() and offset()? Casting 2 hamburgers to integer makes it a valid value: 2.

Should we at least ensure is_numeric()? so that false/null doesn't get cast to 0?

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?

@ralphschindler
Copy link
Member

How would you handle a situation like my example with the string 2 hamburgers? In your opinion that should be a correct value for limit() and offset()? Casting 2 hamburgers to integer makes it a valid value: 2.

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.

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?

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.

@ghost ghost assigned ralphschindler Jun 6, 2013
@ralphschindler
Copy link
Member

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?

@pauloelr
Copy link
Contributor Author

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()
@pauloelr
Copy link
Contributor Author

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.

@Martin-P
Copy link
Contributor

Personally, I would make the exception message more informative than just Invalid value for Limit.. With some extra information you make things much clearer for developers:

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

@pauloelr
Copy link
Contributor Author

Travis is still returning Code Standards error but in a Zend/Crypt/Password/Bcrypt.php and i didn't change this file.
I'm assuming is a Travis issue, not mine, am i right?

@weierophinney
Copy link
Member

@pauloelr The CS issue is not yours; don't worry about it at this time. (I've fixed it.)

@ralphschindler
Copy link
Member

Awesome, looks pretty good, I'll add a unit tests and merge, thanks @pauloelr !

__METHOD__,
(is_object($limit) ? get_class($limit) : gettype($limit))
));
}
Copy link
Member

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.

@mwillbanks
Copy link
Contributor

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.

@ralphschindler
Copy link
Member

I know about this and will make similar changes (as we discussed @mwillbanks) to the MySqlPlatform Sql class.

ralphschindler pushed a commit that referenced this pull request Jun 11, 2013
Merge branch 'zend_db_select_cast_limit_and_offset' of git://github.com/pauloelr/zf2 into pauloelr-zend_db_select_cast_limit_and_offset
ralphschindler pushed a commit that referenced this pull request Jun 11, 2013
Merge branch 'pauloelr-zend_db_select_cast_limit_and_offset'
@ralphschindler ralphschindler merged commit e662a0f into zendframework:master Jun 11, 2013
ralphschindler pushed a commit that referenced this pull request Jun 11, 2013
Merge branch 'pauloelr-zend_db_select_cast_limit_and_offset' into develop
@pauloelr pauloelr deleted the zend_db_select_cast_limit_and_offset branch June 13, 2013 22:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.