Passing a node ID from a lot of sources leads to uc_product_is_product() failing because its check on nid input is in_int(), not is_numeric().

node_load(), which has a similar pattern, does is_numeric().

See http://php.net/manual/en/function.is-int.php:

Note: To test if a variable is a number or a numeric string (such as form input, which is always a string), you must use is_numeric().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

univate’s picture

Status: Active » Needs review
FileSize
427 bytes

Here's a patch, seems to make sense to me.

TR’s picture

Status: Needs review » Needs work

The problem with just using is_numeric() is that then 1.1 would be a valid node number, for example. Also, this would preclude using a numerical value as the name of a product class (that's not too big a deal as a restriction, except for the potential to break existing sites). The analogy with node_load() isn't completely applicable, because uc_product_is_product() accepts either 1) a node number (int), 2) a node type name (string), or 3) a node object (object). You have to figure out a patch that deals with all of these. Perhaps test for int first, since that will be the most common case, then check for string and if the string doesn't match an existing type then check if the string is_numeric(), then finally check for object. Fail if it's none of these.

univate’s picture

PHP has very poor variable typing, which is why using a function which identifies by a type like is_int is always going to be dangerous because everyone else using the function is probably not concerned about ensuring their variables are always properly typed.

You are correct, is_numeric() will return true on a number of different inputs that are not valid node id's including decimal (1.1), exponential (1e3) and hexidecimal (0xAB)

I think the likelihood of those conflicting with a product type are just as likely as a integer - 123 which would be a valid node id and incorrectly interpreted as a node id.

I think it is more likely that you might pass as an argument "123" to this function which would not be interpreted as a node id and instead by seen as a product type because its variable type is a string.

The issue of it conflicting with a node object will not happen as that is an object. So the only question is will a numeric value of decimal, exponential or hexadecimal every be a valid product type?

Personally this type of function highlights one of the biggest issues I have with PHP - its poor typing. There really should be separate functions for this purpose not one functions that tries to deal with different arguments.

longwave’s picture

Version: 6.x-2.x-dev » 7.x-3.x-dev
Status: Needs work » Needs review

#3 makes sense to me. Let's see what testbot thinks.

Status: Needs review » Needs work

The last submitted patch, ubercart-753964.patch, failed testing.

longwave’s picture

Status: Needs work » Needs review
FileSize
435 bytes
longwave’s picture

Status: Needs review » Fixed

Committed to both branches.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.