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.5] add date and cast check for dirty #18400

Merged
merged 8 commits into from Apr 6, 2017

Conversation

Projects
None yet
3 participants
@golubkovden
Copy link
Contributor

golubkovden commented Mar 18, 2017

Reopened PR from #18385

add tests

denis and others added some commits Mar 17, 2017

@GrahamCampbell GrahamCampbell changed the title [5.4] add date and cast check for dirty - reopen [5.5] add date and cast check for dirty - reopen Mar 18, 2017

@GrahamCampbell GrahamCampbell changed the title [5.5] add date and cast check for dirty - reopen [5.5] add date and cast check for dirty Mar 19, 2017

@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Mar 23, 2017

Your tests fail.

@golubkovden

This comment has been minimized.

Copy link
Contributor Author

golubkovden commented Mar 23, 2017

Hm...
Looks like i have same problem with travis as #18417
Will check in local tests.

// When check rich this check and current attribute value not equals with original, we should skip next steps
// if current is null
if (is_null($current)) {

This comment has been minimized.

@golubkovden

golubkovden Mar 23, 2017

Author Contributor

This check for nullable may be should move to fromDateTime method as like castAttribute method.

This hack check looked from setAttribute

This comment has been minimized.

@daftspunk

daftspunk May 13, 2017

Contributor

Fully agree, either that or should to include the original value in the check too:

 if (is_null($current) || is_null($original)) {
  • is_null($current) covers updating filled column with NULL
  • is_null($original) covers updating NULL with filling

@taylorotwell taylorotwell merged commit 89b7a52 into laravel:master Apr 6, 2017

2 checks passed

continuous-integration/styleci/pr The StyleCI analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.