New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eloquent\Model->getDirty() is type sensitive. Is this the behaviour we want? #1429

Closed
ChrisReid opened this Issue May 28, 2013 · 9 comments

Comments

Projects
None yet
4 participants
@ChrisReid

ChrisReid commented May 28, 2013

When a form is submitted, integers are presented as strings. When we resubmit the same form, $value !== $this->original[$key] so is classed as 'dirty' and rewritten to the database. I propose to change this to:

$value != $this->original[$key]

on line 2158. I would submit a patch but PR seems to be disabled.

Can anyone think of why it should be type sensitive?

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell May 28, 2013

Member

I think this has shifted back and forth several times. It seems like some people were changing null to 0 and not checking with type sensitivity was not picking up the change.

Member

taylorotwell commented May 28, 2013

I think this has shifted back and forth several times. It seems like some people were changing null to 0 and not checking with type sensitivity was not picking up the change.

@ChrisReid

This comment has been minimized.

Show comment
Hide comment
@ChrisReid

ChrisReid May 28, 2013

Yeah, I can see a good reason for why you'd want to differentiate 0 and null. I wonder if there's any sense in having the INPUT class convert HTTP post and get numeric values to integer? Does that have any downsides?

ChrisReid commented May 28, 2013

Yeah, I can see a good reason for why you'd want to differentiate 0 and null. I wonder if there's any sense in having the INPUT class convert HTTP post and get numeric values to integer? Does that have any downsides?

@crynobone

This comment has been minimized.

Show comment
Hide comment
@crynobone

crynobone May 28, 2013

Contributor

@chrisreiduk What if I posted a phone number "012-345 6789" or postcode "00123", how does this numeric values to integer handles it?

// manual way would be something like
Input::replace(array('id' => intval(Input::get('id'))));
Contributor

crynobone commented May 28, 2013

@chrisreiduk What if I posted a phone number "012-345 6789" or postcode "00123", how does this numeric values to integer handles it?

// manual way would be something like
Input::replace(array('id' => intval(Input::get('id'))));
@ChrisReid

This comment has been minimized.

Show comment
Hide comment
@ChrisReid

ChrisReid May 28, 2013

@crynobone. Yeah that would break things; trimming off the leading zeros for starters. So not a good idea then.

It's just a shame that PDO and other DB drivers return strings for integer fields.

ChrisReid commented May 28, 2013

@crynobone. Yeah that would break things; trimming off the leading zeros for starters. So not a good idea then.

It's just a shame that PDO and other DB drivers return strings for integer fields.

@bencorlett

This comment has been minimized.

Show comment
Hide comment
@bencorlett

bencorlett May 28, 2013

Contributor

Yeah, t is hey. But at least you can use getter and setter mutators to prepare data for strict comparison.

Contributor

bencorlett commented May 28, 2013

Yeah, t is hey. But at least you can use getter and setter mutators to prepare data for strict comparison.

@ChrisReid

This comment has been minimized.

Show comment
Hide comment
@ChrisReid

ChrisReid May 28, 2013

Maybe have

protected $integers = array('id', 'customer_id' ... etc);

and then logic within Eloquent\Model for automatic integer mutators like we have with the dates. Or anything that ends in '_id' or 'Id' could convert to/from integer?

ChrisReid commented May 28, 2013

Maybe have

protected $integers = array('id', 'customer_id' ... etc);

and then logic within Eloquent\Model for automatic integer mutators like we have with the dates. Or anything that ends in '_id' or 'Id' could convert to/from integer?

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell May 28, 2013

Member

All it would really take on your model would be something like:

public function getAttribute($key)
{
     $value = parent::getAttribute($key);

     if (in_array($key, $this->integers))
     {
          $value = (int) $value;
     }

     return $value;
}
Member

taylorotwell commented May 28, 2013

All it would really take on your model would be something like:

public function getAttribute($key)
{
     $value = parent::getAttribute($key);

     if (in_array($key, $this->integers))
     {
          $value = (int) $value;
     }

     return $value;
}
@bencorlett

This comment has been minimized.

Show comment
Hide comment
@bencorlett

bencorlett May 28, 2013

Contributor

You don't want automatic, not for everybody, or are you just referring within your model?
On 28/05/2013, at 3:10 PM, Chris Reid notifications@github.com wrote:

Maybe have

protected $integers = array('id', 'customer_id' ... etc);
and then logic within Eloquent\Model for automatic integer mutators like we have with the dates. Or anything that ends in '_id' or 'Id' could convert to/from integer?


Reply to this email directly or view it on GitHub.

Contributor

bencorlett commented May 28, 2013

You don't want automatic, not for everybody, or are you just referring within your model?
On 28/05/2013, at 3:10 PM, Chris Reid notifications@github.com wrote:

Maybe have

protected $integers = array('id', 'customer_id' ... etc);
and then logic within Eloquent\Model for automatic integer mutators like we have with the dates. Or anything that ends in '_id' or 'Id' could convert to/from integer?


Reply to this email directly or view it on GitHub.

@ChrisReid

This comment has been minimized.

Show comment
Hide comment
@ChrisReid

ChrisReid May 28, 2013

Taylor's solution above into my BaseModel works for me! Thanks

ChrisReid commented May 28, 2013

Taylor's solution above into my BaseModel works for me! Thanks

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