Skip to content
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

[5.8] Set Numeric value as Model Attribute #29714

Merged
merged 5 commits into from
Aug 25, 2019
Merged

[5.8] Set Numeric value as Model Attribute #29714

merged 5 commits into from
Aug 25, 2019

Conversation

me-shaon
Copy link
Contributor

If someone tries to set a Numeric value as a Model attribute, it will fail because of isDateAttribute methods loose comparison in in_array searching.
The details described in reported issue.
Simply casting all attributes to string in setAttribute method will resolve the issue.

This PR fixes issue #29702

@GrahamCampbell GrahamCampbell changed the title [fix]: #29702 Set Numeric value as Model Attribute [5.8] Set Numeric value as Model Attribute Aug 24, 2019
@@ -562,6 +562,7 @@ protected function isDecimalCast($cast)
*/
public function setAttribute($key, $value)
{
$key = (string) $key;
Copy link
Member

Choose a reason for hiding this comment

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

The phpdoc says that the key is already a string, so any code not passing a string to this function needs to be fixed, instead, and then this line removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Agree with that. I guess I should just cast string at the __set method in Model class like the following, right?

public function __set($key, $value)
{
      $this->setAttribute((string) $key, $value);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although, there are a lot of other setAttribute method calls. Do you think all of them should cast string on their keys too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GrahamCampbell I've updated the PR by changing the implementation as Suggested in the Issue. As casting the $key to string in every setAttribute call doesn't seem like a good solution to me.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. Agree with that. I guess I should just cast string at the __set method in Model class like the following, right?

Any dynamic call to that method will already provide a non-empty string as the key. No need to cast.

@taylorotwell taylorotwell merged commit 6fd0d16 into laravel:5.8 Aug 25, 2019
@me-shaon me-shaon deleted the patch/issue-29702 branch August 25, 2019 15:53
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.

3 participants