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

Returning Carbon instances when using Model Casting #35

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

amandiobm
Copy link
Contributor

@amandiobm amandiobm commented May 19, 2020

Custom Cast - https://laravel.com/docs/8.x/eloquent-mutators#custom-casts

Add a new way to handle user timezone dates.

When used on the Eloquent, it automatically transforms the value into a Carbon instance with the user timezone applied.

@jpmurray
Copy link

I'm a huge fan of your work here! 😂 Quick look around doens't raise any concern, sounds good! 👍

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@jpmurray
Copy link

@jamesmills any idea if you want to merge this or not ?

@jamesmills jamesmills added enhancement New feature or request question Further information is requested labels Feb 18, 2021
@jamesmills
Copy link
Owner

Sorry, late to the party @jpmurray

I do like the look of this.

Forgive me, I've been out of the Laravel world for a while. Is the addition of this going to break the default usage or is it just an added optional feature? I cannot remember what the Casts thing actually does but my initial thinking is it's just an added additional feature that can be used or ignored?

Also love the trait TimezoneTrait -> protected function getUserTimezone refactor @amandiobm ;-)

Let's get this in!

@amandiobm
Copy link
Contributor Author

Updated the description.

@jamesmills
Copy link
Owner

So this would remove all the need for us to format the date/time because it would just be an instance of Carbon with the correct timezone set. I love this. Maybe what I should have done with the main package....

@jedjones-uk
Copy link

I think there may be an issue with the logic here. When using the cast on the updated at attribute, the set function is called through touch() which creates a new Carbon object via ::now(). This object will have the current timezone of your app, or server, rather than the local timezone. This current object/timezone will then be passed through the convertFromLocal method which will re-parse that object as your local timezone and convert it again to UTC.

The end result is, if for example you are currently UTC+1 (UK daylight savings) but your server is configured for UTC, if you perform an action at 10:30am, the touch timestamp will be 9:30am, and after convert from local it will be recorded at 8:30am.

As far as I can tell, the set function doesn't provide enough info to determine if it was real local timezone, or one of these server generated ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants