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

3.0 - Extend DateTime to add a __toString and use that class instead when hydrating #2613

Closed
lorenzo opened this issue Jan 6, 2014 · 16 comments

Comments

@lorenzo
Copy link
Member

lorenzo commented Jan 6, 2014

According to some feedback I've got for 3.0-dev, doing echo $entity->created; throws an exception since the date object cannot be converted to string. Creating a class that extends DateTime and adding a __toString method to it will solve this issue and make the views more visually appealing.

We may consider adding some formatting shortcuts to it too.

@jippi
Copy link
Contributor

jippi commented Jan 6, 2014

Sounds like CakeTime :)

@lorenzo
Copy link
Member Author

lorenzo commented Jan 6, 2014

Only that CakeTime does not extend DateTime and all the methods are static.

@jippi
Copy link
Contributor

jippi commented Jan 6, 2014

I'm aware, but imo it would make sense to make CakeTime extend DateTime and make methods non static where it makes sense. e.g. TimeAgoInWords and all those do need to be static, they need a time anyway :)

some of the methods can remain static, while other might as well work on an instance :)

@markstory
Copy link
Member

How would someone set the default format for this class?

@jippi
Copy link
Contributor

jippi commented Jan 6, 2014

like today, either make them remain static or by passing it in through the constructor / set method

@lorenzo
Copy link
Member Author

lorenzo commented Jan 6, 2014

@markstory Im thinking of using the Type system. You could do Type::build('date')->setDefaultFormat('Y-m-d') static properties and Entity getters are the other options

@markstory
Copy link
Member

@jippi I kind of like having Cake\Utility\Time be an extension of DateTime. I bet there are some interesting ideas hidden in doing that.

@bcrowe
Copy link
Contributor

bcrowe commented Jan 11, 2014

After looking into / beginning to have Time extend DateTime, I wasn't seeing much value. For the most part, it's all static methods and there isn't much value outside of not having to instantiate a DateTime object inside of these methods. In my experiences, Time's role is primarily serving as the engine for the TimeHelper. (That's subject to myself though.) That being said, after bouncing this off of @markstory, he suggested having the ORM returning an improved DateTime object for datetime fields. Ex: $post->created->timeAgoInWords();

Mark, please chime in if I've overlooked or misunderstood anything.

@markstory
Copy link
Member

Nope that was my thinking too. This would also allow us to provide a useful __toString() method allowing <?= $post->created; ?> to do something useful.

@lorenzo
Copy link
Member Author

lorenzo commented Jan 12, 2014

Maybe we can just use an external lib for this? https://github.com/briannesbitt/Carbon

@markstory
Copy link
Member

Using carbon might save us a bunch of time and effort. It has a pretty nice interface and the testing features are a bonus.

@lorenzo
Copy link
Member Author

lorenzo commented Jan 12, 2014

Yeah, I particularly like the testing features which would solve one the nightmares from 2.x

@jippi
Copy link
Contributor

jippi commented Jan 13, 2014

👍 for Carbon :=)

@markstory
Copy link
Member

Not only does it scratch our own itch, but it sets up application developers better as well. They too can benefit for the easier testing that carbon provides.

@markstory
Copy link
Member

Can this be closed with the Carbon dependency being used now?

@lorenzo
Copy link
Member Author

lorenzo commented Apr 18, 2014

Yes

@lorenzo lorenzo closed this as completed Apr 18, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants