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 - Replace the DateTime object #3328

Merged
merged 8 commits into from Apr 17, 2014
Merged

3.0 - Replace the DateTime object #3328

merged 8 commits into from Apr 17, 2014

Conversation

@lorenzo
Copy link
Member

lorenzo commented Apr 15, 2014

Implements #2613

@josegonzalez
Copy link
Member

josegonzalez commented Apr 15, 2014

@dereuromark dereuromark added this to the 3.0.0 milestone Apr 15, 2014
@lorenzo
Copy link
Member Author

lorenzo commented Apr 15, 2014

@josegonzalez what a troll

@dereuromark
Copy link
Member

dereuromark commented Apr 15, 2014

Yeah ;) Terrible..^^

Nice addition, @lorenzo !

@@ -20,7 +20,8 @@
"require": {
"php": ">=5.4.19",
"ext-mcrypt": "*",
"ext-mbstring": "*"
"ext-mbstring": "*",
"nesbot/Carbon": "1.*"

This comment has been minimized.

Copy link
@markstory

markstory Apr 16, 2014

Member

We might want to pin to a more specific version. I personally always pin to exact versions, as I have been burned too many times.

This comment has been minimized.

Copy link
@josegonzalez

josegonzalez Apr 16, 2014

Member

👍 with a cautious 👍 to pinning to a minor release.

@markstory
Copy link
Member

markstory commented Apr 16, 2014

What kind of impact will Carbon have on CakeTime? Does CakeTime still have a reason to continue being included in cake? Should CakeTime be updated to bettee utilise Carbon?

@dereuromark
Copy link
Member

dereuromark commented Apr 16, 2014

I am a little bit cautious after looking into it more - it seems the project is dormant and not actively maintained (especially regarding bugfixes) for 3+ months now.
Just saying.. I would have expected a little bit more from such a popular and well used lib.

@ADmad
Copy link
Member

ADmad commented Apr 16, 2014

Does CakeTime still have a reason to continue being included in cake? Should CakeTime be updated to bettee utilise Carbon?

Having CakeTime and Carbon both does seem redundant. If we can't get rid of CakeTime all together it should be updated to utilize Carbon.

it seems the project is dormant and not actively maintained (especially regarding bugfixes) for 3+ months now.

3 months doesn't seem too long for a single class lib about 2 years old. It should be stable enough to not require frequent updates / fixes.

@ADmad
Copy link
Member

ADmad commented Apr 16, 2014

Customary note about CS errors :)

@lorenzo
Copy link
Member Author

lorenzo commented Apr 16, 2014

My idea, since I proposed using carbon, was to make CakeTime a wrapper of Carbon for backwards compatibility an adding our own features. Now that I think about more about it, in order to shield ourselves against lack of activity in Carbon, we can just use CakeTime internally after having it extend Carbon. If we decide to fork the other project, fix issues or add new functionality we can do it directly in CakeTime.

Is that reasonable?

@ADmad
Copy link
Member

ADmad commented Apr 16, 2014

Sounds reasonable. It's a good path to follow in general too for external libs we might use in future too. We should have our own classes extend / wrap the lib so we can change the guts as needed if required.

@lorenzo
Copy link
Member Author

lorenzo commented Apr 16, 2014

I will do that, Including updating CakeTime, in another PR if all agree.

@dereuromark
Copy link
Member

dereuromark commented Apr 16, 2014

Sounds good 👍 You meant updating CakeTime => updating Time though probably.

@ceeram
Copy link
Member

ceeram commented Apr 16, 2014

I object for no good reasons, but in absence of @rchavik ill object for him :)

@rchavik
Copy link
Member

rchavik commented Apr 17, 2014

Naaaaeeeeeooooooooooooooooooooouuuu!!!

I object, you can already use Carbon without us adding it as a dependency on the framework level. I even don't like having CakeTime as proxy for Carbon. Just let users use Carbon directly instead.

I'm all for marking CakeTime as @deprecated instead.

@josegonzalez
Copy link
Member

josegonzalez commented Apr 17, 2014

@rchavik why would we do either:

  • not use carbon
  • just mark all of caketime as deprecated
@bcrowe
Copy link
Contributor

bcrowe commented Apr 17, 2014

👍 I like the plan.

@josegonzalez
Copy link
Member

josegonzalez commented Apr 17, 2014

Not to sound like an ass, I just want to know if there is a reason why we would not want external libraries in our framework.

After discussing with @rchavik, the reason for not doing so appears to be that CakePHP is currently a drop-in framework - something where the framework doesn't have external dependencies. It also seems that @rchavik does not find it preferable to split up CakePHP into multiple, separate sub-projects - CakePHP/Model, CakePHP/Controller, CakePHP/View, etc. - for similar reasons.

I will say that I am 👍 on the merge, with the caveat that I do not maintain the cakephp core framework and thus don't know who it is we are attempting to attract with our framework. If it is that we want to lower the barrier to entry, then anything that would require a separate repository of any kind is a no-go.

I also think that creating our own constructs for everything is a bad idea. I think it's something that will inundate those who maintain the framework, and force us to rewrite things that are already written, maintained, tested, etc. The purpose of the framework is to make things as DRY as possible, and I think that goal is at odds with not using external libraries.

@josegonzalez
Copy link
Member

josegonzalez commented Apr 17, 2014

@rchavik feel free to correct me if I portrayed your thoughts on the matter incorrectly :)

@ADmad
Copy link
Member

ADmad commented Apr 17, 2014

If it is that we want to lower the barrier to entry, then anything that would require a separate repository of any kind is a no-go.

I don't see how using external libs raises the barrier to entry for new users. Regardless of whether we use external libs or not for the end user it's still a single composer install command.

I will say that I am 👍 on the merge, with the caveat that I do not maintain the cakephp core framework and thus don't know who it is we are attempting to attract with our framework.

Framework maintenance is the key issue here. Currently @markstory and @lorenzo do the bulk of the work. Rest of the team helps wherever they can but they are the driving force. I am sure they would appreciate not having to maintain every single package. If the lead devs think using external libs should be used where necessary / feasible we should support them. It's not like we are picking components off the net and just gluing them together to make our framework. Libs are selected just for specific tasks.

Look at Kohana as an example of what happens when the lead devs are no longer able to maintain a drop-in framework.

I think it's something that will inundate those who maintain the framework, and force us to rewrite things that are already written, maintained, tested, etc. The purpose of the framework is to make things as DRY as possible, and I think that goal is at odds with not using external libraries.

Exactly, there's no point reinventing the wheel. Surely the maintainers of the framework can find better use of their time than rewriting stuff that's already available.

tldr; I appreciate @rchavik's point of view but it's time to accept change :)

lorenzo added a commit that referenced this pull request Apr 17, 2014
3.0 - Replace the DateTime object
@lorenzo lorenzo merged commit 7f84544 into 3.0 Apr 17, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@lorenzo lorenzo deleted the 3.0-carbon branch Apr 17, 2014
@bcrowe bcrowe restored the 3.0-carbon branch Apr 17, 2014
@bcrowe bcrowe deleted the 3.0-carbon branch Apr 17, 2014
@markstory
Copy link
Member

markstory commented Apr 18, 2014

I think we can still have some of the 'drop in' ness by providing good tarballs/zipped packages. We've often talked about having them for the app skeleton, would it also be useful to have tarballs for just the framework?

@rchavik
Copy link
Member

rchavik commented Apr 18, 2014

Not worth the hassle IMO.

@kicaj

This comment has been minimized.

Copy link
Contributor

kicaj commented on e78f408 Apr 23, 2014

Can You update installation.rst (https://github.com/cakephp/docs/blob/3.0/en/installation.rst) in Requirements paragraph? Can You write something about Carbon, how to install (manualy), where and why? Which: https://packagist.org/packages/nesbot/carbon or https://github.com/briannesbitt/Carbon? Now without this I can't test 3.0 (latest commitions), thanks a lot!

This comment has been minimized.

Copy link
Member

dereuromark replied Apr 23, 2014

They are the same thing.
All you need to do is: composer update.

This comment has been minimized.

Copy link
Member

ADmad replied Apr 23, 2014

@kicaj We intend to only support using composer as the official way to install CakePHP and it's dependencies. Supporting multiple ways makes our job of maintaining the code and docs lot more tedious. The docs are being updated to remove mention of alternate install methods.

This comment has been minimized.

Copy link
Member

markstory replied Apr 23, 2014

I'll get the docs updated tonight.

This comment has been minimized.

Copy link
Member

ADmad replied Apr 23, 2014

@markstory I already opened PRs for that, you just have to hit the merge button 😄

This comment has been minimized.

Copy link
Member

markstory replied Apr 23, 2014

Yeah you beat me to it, nice work 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.