Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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)
Comment | File | Size | Author |
---|---|---|---|
#17 | 487232-use-filter-var-in-core-validation-rev4.patch | 12.45 KB | brianV |
Comments
Comment #1
brianV CreditAttribution: brianV commentedI'll take this one :p
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedI think it's time for a drupal_is_integer() or similar.
Comment #3
brianV CreditAttribution: brianV commentedIs 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.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedI commented about #3 in #2.
Comment #5
brianV CreditAttribution: brianV commentedDamien,
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:
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().
Comment #6
brianV CreditAttribution: brianV commentedAfter much booing and hissing in #drupal, let's just leave drupal_validate_integer() simple:
drupal_validate_integer($value)
Comment #7
brianV CreditAttribution: brianV commentedUpdating 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.
Comment #8
chx CreditAttribution: chx commentedThanks 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.
Comment #9
brianV CreditAttribution: brianV commentedchx,
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.
Comment #10
brianV CreditAttribution: brianV commentedComment #11
catchI reckon if you wanted a range you could use filter_var() directly. This looks like good cleanup.
Comment #12
Dries CreditAttribution: Dries commentedThis looks good to me. The difference between writing
drupal_validate_float($value)
andfilter_var($value, FILTER_VALIDATE_FLOAT)
is minimal butdrupal_validate_float($value)
is probably a bit more readable.Comment #13
brianV CreditAttribution: brianV commentedJust 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.Comment #14
Damien Tournoud CreditAttribution: Damien Tournoud commentedI 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:
!== FALSE
. Please also modify drupal_validate_url() for consistency.Comment #15
brianV CreditAttribution: brianV commentedGood 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.
Comment #17
brianV CreditAttribution: brianV commenteddoh - that's embarassing.
Comment #18
Dries CreditAttribution: Dries commentedI'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.
Comment #19
brianV CreditAttribution: brianV commented@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.
Comment #20
ekes CreditAttribution: ekes commentedchx said:
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:
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:-
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.
Comment #21
Damien Tournoud CreditAttribution: Damien Tournoud commentedNote 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.Comment #22
ekes CreditAttribution: ekes commentedDoh! 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:-
is returning false.
Maybe FILTER_SANITIZE_NUMBER_INT is more appropriate there then?
Comment #23
mfer CreditAttribution: mfer commented@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.
Comment #24
brianV CreditAttribution: brianV commented@mfer,
That is exactly what the link I posted in #13 explained :P
Comment #25
mfer CreditAttribution: mfer commentedComment #26
jhedstromComment #27
andypostd8 using symfony validators so no idea what's to do here
Maybe replace usage of
parse_url()
withfilter_var(FILTER_VALIDATE_URL)
Comment #28
andypostComment #29
vijaycs85#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)"
Comment #41
andypost