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.

Sign up for GitHub

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

Jump to bottom

fix: deleted_at column has date type rather than string #1124

Merged
merged 11 commits into from
Mar 2, 2022
Merged

fix: deleted_at column has date type rather than string #1124

merged 11 commits into from
Mar 2, 2022

Conversation

totov
Copy link
Contributor

@totov totov commented Feb 10, 2022

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Resolves #1051

Changes

This updates the SchemaAggregator to read SoftDeletes and SoftDeletesTz columns as Carbon instances instead of strings.

@szepeviktor
Copy link
Collaborator

Thank you @totov!

I'm not sure about Carbon/Carbon vs. Illuminate\Support\Carbon
@canvural Could you help?

@BertvanHoekelen
Copy link
Contributor

Nice! I think you should get the date type from get_class(\Illuminate\Support\Facades\Date::now()) as you can have overwritten the default Date with Date::use(CarbonImmutable::class) in a service provider.

@totov
Changes Carbon instance to use the Date Facade to ensure it retur…
7c39a99 
…ns the instance which can set by the user in the service provider.
@totov
Copy link
Contributor Author

totov commented Feb 15, 2022

Is there anything I can do to improve this PR?

@szepeviktor
Copy link
Collaborator

Is there anything I can do to improve this PR?

I think no, you can't.
We need a merging hand.

canvural
canvural requested changes Mar 2, 2022
View reviewed changes
Copy link
Collaborator

@canvural canvural left a comment

Choose a reason for hiding this comment

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

Hi,

Sorry for the late response.

It looks ok. Even though I don't like the get_class(Date::now)) call, for now it's fine I guess.

src/Properties/SchemaAggregator.php Outdated Show resolved Hide resolved
@totov totov requested a review from canvural March 2, 2022 11:56
@totov
Copy link
Contributor Author

totov commented Mar 2, 2022

I've submitted a few commits with changes to the PR in line with the earlier discussion. Let me know if I can do anything else to improve this.

canvural
canvural requested changes Mar 2, 2022
View reviewed changes
Copy link
Collaborator

@canvural canvural left a comment

Choose a reason for hiding this comment

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

This looks good. One small nitpick I wrote. After that we can merge!

src/Properties/ModelPropertyExtension.php Outdated Show resolved Hide resolved
@canvural canvural changed the title Changes type of deleted_at columns to be Carbon instance fix: deleted_at column has date type rather than string Mar 2, 2022
@canvural canvural merged commit 6caf3f9 into larastan:master Mar 2, 2022
@canvural
Copy link
Collaborator

canvural commented Mar 2, 2022

Looks good. Thank you!

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

@canvural canvural canvural requested changes

@szepeviktor szepeviktor szepeviktor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

With SoftDeletes deleted_at is a Carbon instance, not a string
4 participants
@totov @szepeviktor @BertvanHoekelen @canvural