Skip to content

Comments

[8.x] Add integer() method to InteractsWithInput trait#39714

Closed
BrendanKoral wants to merge 2 commits intolaravel:8.xfrom
BrendanKoral:feature/retrieve-request-input-as-integer
Closed

[8.x] Add integer() method to InteractsWithInput trait#39714
BrendanKoral wants to merge 2 commits intolaravel:8.xfrom
BrendanKoral:feature/retrieve-request-input-as-integer

Conversation

@BrendanKoral
Copy link

This PR is inspired by the boolean() method on the InteractsWithInput trait. I often find that I have to write something like this when reading numbers out of a request:

$myVal = (int)$request->query('myval');

This PR eliminates the typecasting by adding an integer() method similar to the boolean() method.

$myVal = $request->integer('myval');

If there's already an existing way to do something like this and I've overlooked it, please let me know. I didn't see anything like this in the documentation.

@oliverquynh
Copy link
Contributor

The $query and $request properties of the $request object are instances of Symfony\Component\HttpFoundation\ParameterBag, so you can use the getInt method of it like this:

$request->query->getInt('your var');
$request->request->getInt('your_var');

Anyway, \Illuminate\Http\Request class already has a method called boolean. It depends, these proxy methods may be helpful or I can just cast values manually, it's fine.

*
* Returns an integer when value is a boolean or a number in string form i.e. "123". Otherwise, returns false.
*
* @param null $key
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

incorrect type

* Returns an integer when value is a boolean or a number in string form i.e. "123". Otherwise, returns false.
*
* @param null $key
* @param false $default
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@GrahamCampbell GrahamCampbell changed the title Add integer() method to InteractsWithInput trait [8.x] Add integer() method to InteractsWithInput trait Nov 21, 2021
@derekmd
Copy link
Contributor

derekmd commented Nov 21, 2021

integer() never returns bool

The parameter $default = false makes sense for Request@boolean() that always returns bool but may not for a new integer() method. The intention here seems to be coercing to single type yet it may return a mix.

Request::create('/', 'GET', [])->integer('foo');
// false

Request::create('/', 'GET', ['foo' => ''])->integer('foo');
// false

Request::create('/', 'GET', ['foo' => 'bar'])->integer('foo');
// false

To support passing the integer() return value into a method with type-checked parameter ?int $arg or to fill in a strict-type database column, this new method should probably fallback to null instead of false.

public function integer($key = null, $default = null)
{
    return filter_var($this->input($key, $default), FILTER_VALIDATE_INT, FILTER_NULL_ON_FAILURE);
}

Casting vs. filter_var()

Now comparing the filter_var() implementation and (int) casting:

use Illuminate\Support\Facades\Request;
Request::macro('integer', function ($key = null, $default = null) {
    return filter_var($this->input($key, $default), FILTER_VALIDATE_INT, FILTER_NULL_ON_FAILURE);
});

Request::create('/', 'GET', [])->integer('foo');
// null
Request::create('/', 'GET', [])->query->getInt('foo');
// 0

Request::create('/', 'GET', ['foo' => ''])->integer('foo');
// null
Request::create('/', 'GET', ['foo' => ''])->query->getInt('foo');
// 0

// I assume request validation makes the next 4 use cases irrelevant:

Request::create('/', 'GET', ['foo' => 'bar'])->integer('foo');
// null
Request::create('/', 'GET', ['foo' => 'bar'])->query->getInt('foo');
// 0

Request::create('/', 'GET', ['foo' => '1.234'])->integer('foo');
// null
Request::create('/', 'GET', ['foo' => '1.234'])->query->getInt('foo');
// 1

Request::create('/', 'GET', ['foo' => '1bar'])->integer('foo');
// null
Request::create('/', 'GET', ['foo' => '1bar'])->query->getInt('foo');
// 1

Request::create('/', 'GET', ['foo' => 'bar2'])->integer('foo');
// null
Request::create('/', 'GET', ['foo' => 'bar2'])->query->getInt('foo');
// 0

// integer disallows leading zeroes? see below
Request::create('/', 'GET', ['minute' => '07'])->integer('minute');
// null
Request::create('/', 'GET', ['minute' => '07'])->query->getInt('minute');
// 7

Request::create('/', 'GET', ['minute' => '7'])->integer('minute');
// 7
Request::create('/', 'GET', ['minute' => '7'])->query->getInt('minute');
// 7

So casting and filter_var() have some differing behavior that may make the filter_var() implementation more trouble than its worth although it does match the integer validation rule:

public function validateInteger($attribute, $value)
{
return filter_var($value, FILTER_VALIDATE_INT) !== false;
}

e.g., edge case for input of minute/hour with a leading 0, which client-side validation <input type="number"> allows:

filter_var('07', FILTER_VALIDATE_INT, FILTER_NULL_ON_FAILURE)
// null

filter_var('07', FILTER_VALIDATE_INT, FILTER_NULL_ON_FAILURE | FILTER_FLAG_ALLOW_OCTAL)
// 7

Userland solution for handling base 10 integer input

I found the best catch-all solution was avoiding filter_var() by validating with a regex and then casting:

class BaseTenInteger implements \Illuminate\Contracts\Validation\Rule
{
    public function passes($attribute, $value)
    {
        return preg_match('/^-?[0-9]+$/', $value);
    }

    public function message()
    {
        return trans('validation.integer');
    }
}

Required integer parameter

request()->validate(['foo' => ['required', new BaseTenInteger]);
$foo = (int) request('foo');

Optional integer parameter

request()->validate(['foo' => new BaseTenInteger]);
$foo = request()->filled('foo')
    ? (int) request('foo')
    : null;

@taylorotwell
Copy link
Member

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants