Most often when Drupal uses is_numeric($foo), it means filter_var($foo, FILTER_VALIDATE_INT, array("options" => array('min_range' => 1)) or similar. is_numeric accepts floats, hex... FILTER_VALIDATE_INT does not.

URL checking logic could be checked with filter_var($url, FILTER_VALIDATE_URL)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

brianV’s picture

Assigned: Unassigned » brianV

I'll take this one :p

Damien Tournoud’s picture

I think it's time for a drupal_is_integer() or similar.

brianV’s picture

Is it worth possibly writing some new functions to wrap filter_var() for readability?

Such as:

drupal_is_integer()
drupal_is_float()
etc. etc.

Each of these new functions would automatically set the corresponding filter constants, appropriate flags and whatnot for the content type they are checking.

I am a little concerned that changing everything to filter_var() calls with the variety of parameters and flags would make the code less readable.

Damien Tournoud’s picture

I commented about #3 in #2.

brianV’s picture

Damien,

That's what I get about thinking too long about a reply before actually typing it in :p

Then I suggest we write the following functions to be included in common.inc:

drupal_validate_integer($value, $min_range = 0, $max_range = NULL, $allow_hex = FALSE, $allow_octal = FALSE)
drupal_validate_float($value)

Also, rename:

valid_email_address() to drupal_validate_email($value)
valid_url() to drupal_validate_url($url, $absolute = FALSE)

to stay in keeping with the new function names. Note - valid_email_address already uses filter_var(). valid_url() would need to be converted to use filter_var().

brianV’s picture

After much booing and hissing in #drupal, let's just leave drupal_validate_integer() simple:

drupal_validate_integer($value)

brianV’s picture

Title: Replace is_numeric with filter_var » Use filter_var() for core validation functions
Status: Active » Needs review
FileSize
13.44 KB

Updating name to something more fitting.

Also, here is an initial patch for review, with the new functions written, and both valid_email and valid_url renamed as per #5.

All core functions and tests have been updated to use the new drupal_validate_url() and drupal_validate_email() functions. I am submitting this mostly to get comments and see how the testbots like it.

The calls to is_numeric() have yet to be updated to the new functions. Also, we need unit tests.

chx’s picture

Status: Needs review » Needs work

Thanks for working on this but please note that in the original post I have said that we need a range -- I would recommend adding a min and maybe a max to the INT validation function. Nice cleanup on the rest.

brianV’s picture

chx,

There was a bit of a debate on #drupal, and the consensus was that a range should not be part of the integer validation. Conversation included Heine, DamZ, Berdir and myself among others.

Anyways, I would like to leave this at 'needs review' until the testbot finishes with it. After that, I will set it to CNW until it's settled whether a range should be included in the validation.

brianV’s picture

Status: Needs work » Needs review
catch’s picture

I reckon if you wanted a range you could use filter_var() directly. This looks like good cleanup.

Dries’s picture

This looks good to me. The difference between writing drupal_validate_float($value) and filter_var($value, FILTER_VALIDATE_FLOAT) is minimal but drupal_validate_float($value) is probably a bit more readable.

brianV’s picture

Just a note, we can't use filter_var() for URL validation. It failed a lot of tests on my local system. A little digging turned up http://www.talkincode.com/php-filter-filter_validate_url-limitations-124..., which matches what I am seeing. I am moving the URL validation back to being regex-based for now.

Damien Tournoud’s picture

Status: Needs review » Needs work

I like. As I said some other place, PHP Filtering is a good idea but failed as the realization (the URL filtering is unusable, for example).

About the patch:

  • The summary of the doxygen comments must fit in one line, per coding standards.
  • If I believe the PHP documentation, filter_var() returns "Returns the filtered data, or FALSE if the filter fails." (which makes no sense at all, I believe), so simply casting to bool will fail for the "0" value. We have to use !== FALSE. Please also modify drupal_validate_url() for consistency.
brianV’s picture

Status: Needs work » Needs review
FileSize
12.39 KB

Good catch, Damien.

I've updated the drupal_validate_email(), drupal_validate_integer() and drupal_validate_float() returns as per #14.

I still need to write unit tests for these functions, and start converting the rest of core to use them, but I will leave this patch up for review and testbot.

Please leave at 'needs review' until the testbot has a chance to play with it.

Status: Needs review » Needs work

The last submitted patch failed testing.

brianV’s picture

Status: Needs work » Needs review
FileSize
12.45 KB

doh - that's embarassing.

Dries’s picture

Status: Needs review » Needs work
Issue tags: +Favorite-of-Dries

I'd like to better understand the future of filter_var() and if the PHP community has a desire to fix their URL validation function, but nonetheless, I'm tagging this. I'll do some research on that, but feel free to point me, and others, in the right direction.

It looks good but we need to write test for this.

brianV’s picture

@Dries: PHP bugs reported about filter_var()'s problems:

All filter-related bugs (37)

It looks like they are trying to fix bugs in the filters... However, unless we *require* a version of PHP > 5.2.6 or something, we aren't likely to get most of the listed fixes. Many aren't in 5.2.0, which AFAIK is the min-required version for D7.

ekes’s picture

chx said:

Most often when Drupal uses is_numeric($foo), it means filter_var($foo, FILTER_VALIDATE_INT, array("options" => array('min_range' => 1)) or similar

So in irc I queried if Drupal shouldn't be casting to int more often, because int was meant. chx replied that the situations were usually actually also strings:

2009-06-10 08:42 <@ chx> ekes: if you pass in a real string then casting to int will be 0
...
2009-06-10 08:43 <@ chx> ekes: we want to check
...
2009-06-10 08:43 <@ chx> ekes: please follow up that in some case we want to do it
...
2009-06-10 08:43 <@ chx> ekes: but i think that's the minority

Leaving me duty bound to actually look :-) So for what it's worth now I've looked, if there it's any help, here's my results. Uses of is_numeric in core @ head:-

487232-is_numeric-mixed - cases where the variable could be an integer or something else, and is used as such
30 uses of is_numeric
  • these were split into more than 1 and could be less than one, because I started at the point of the discussion above
  • several cases of mixed string or int passed into a function could be $int = (int) $mixed, but I don't think it would help readablity.
  • some cases are validation where (int) might work to as they can't be 0, but the string is never actually used as
  • a good number of cases where it's array keys numeric or associative
487232-is_numeric-numeric - cases where checking for a truly numeric value
5 uses of is_numeric
487232-is_numeric-int - cases where the variable goes on to only be used as an int
26 uses of is_numeric
  • I've often written in the note the casting alternative, that's not to say it would be more readable, or efficient, than using filter - I don't know - just the variables are only used as int

The attached files are just grep with filenames, line numbers and a line either side of the use of is_numeric and with my comment added. I've really only just looked so you might disagree. Never know it might be helpful though, happy to follow-up if I can and it is.

Damien Tournoud’s picture

Note that (int) is 32bit unsigned (on 32bit platforms), while we are generally storing identifiers as INT UNSIGNED in the database engine, which is generally bigger.

ekes’s picture

Doh! Very true (although signed).

The filter_var($var, FILTER_VALIDATE_INT) option for replacing is_numeric falls the same hurdle for bigger identifiers. At least:-

$max = PHP_INT_MAX + 1;
var_dump(filter_var($max, FILTER_VALIDATE_INT));
var_dump(filter_var("$max", FILTER_VALIDATE_INT));

is returning false.

Maybe FILTER_SANITIZE_NUMBER_INT is more appropriate there then?

mfer’s picture

@brianV and @Dries - the url filter really just sees if parse_url can parse the url and doesn't actually validate it. This is neat because parse_url has a note in it's documentation that it is not meant to validate a url. That's why we put the current regex in rather than use filter_var. filter_var for urls fails a lot of the tests we currently have in core.

brianV’s picture

@mfer,

That is exactly what the link I posted in #13 explained :P

mfer’s picture

Version: 7.x-dev » 8.x-dev
jhedstrom’s picture

Issue summary: View changes
Issue tags: +Needs issue summary update
andypost’s picture

d8 using symfony validators so no idea what's to do here
Maybe replace usage of parse_url() with filter_var(FILTER_VALIDATE_URL)

andypost’s picture

Version: 8.0.x-dev » 8.1.x-dev
vijaycs85’s picture

#27: yes, we are using symfony/validator/Constraints/EmailValidator.php which has !preg_match('/^.+\@\S+\.\S+$/', $value). So if we plan for update, it should be on the upstream.

Hope, we can close this issue with "Closed (Won't fix)"

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.