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

Stop using Carbon? #6509

Closed
lorenzo opened this issue May 7, 2015 · 27 comments
Closed

Stop using Carbon? #6509

lorenzo opened this issue May 7, 2015 · 27 comments
Labels
RFC
Milestone

Comments

@lorenzo
Copy link
Member

@lorenzo lorenzo commented May 7, 2015

I went to the Carbon project to see what's new there and found this jewel:

https://github.com/briannesbitt/Carbon/blob/master/composer.json#L21

As it stands now I'm not too happy with our dependency on carbon for several reason:

  • We have failed a communicating the API that is available
  • Bugs are fixed at a slow pace
  • It could definitely be faster
  • Introduces a big chain of dependencies

I also have some ideas of how it could be done better:

  • Move all methods to a trait
  • Implement our own DateTime (and in the future DateTimeImmutable) using the trait
  • Reduce internal method calls that make it slow
  • Offer I18n as a separate thing as we already do.

This will also give us the excuse to properly document the API in our docs.

@lorenzo lorenzo added the RFC label May 7, 2015
@lorenzo lorenzo added this to the 3.1.0 milestone May 7, 2015
@josegonzalez
Copy link
Member

@josegonzalez josegonzalez commented May 7, 2015

Can we offer this as a standalone datetime library? Is there an alternative to carbon that we can use?

@lorenzo
Copy link
Member Author

@lorenzo lorenzo commented May 7, 2015

That was my idea @josegonzalez

@dereuromark
Copy link
Member

@dereuromark dereuromark commented May 7, 2015

Awesome idea. Just what I had in mind from the beginning :) Carbon was a great library, but yeah, way too slow pace and not necessarily going in the right directions. Having a cakephp/carbon, cakephp/time or sth is the way to go 👍

With such a strong community behind it, it will soon outrun carbon as the de-facto standard.

@markstory
Copy link
Member

@markstory markstory commented May 8, 2015

I am ok with forking carbon and optimizing/simplifying the internals. Reimplementing an API compatible alternative is also a good option and would allow us to provide immutable variants like @lorenzo mentioned.

In either scenario we should aim to have a library with 0 dependencies.

@kicaj
Copy link
Contributor

@kicaj kicaj commented May 9, 2015

👍 for @lorenzo idea:)

@jadb
Copy link
Member

@jadb jadb commented May 9, 2015

👍 just because it started relying on other dependencies.

@sukihub
Copy link
Contributor

@sukihub sukihub commented May 9, 2015

👍 for immutable Time (I just hate that I have to ->copy() it every time I want to add an hour or change the timezone or ...). I'm however not a big fan of creating yet another PHP (DateTime) library. We all would benefit from 1 really good DateTime library more than from 5 poor ones. Can't we just contribute to Carbon? Maybe suggest them to split it into two libraries (core and i18n stuff)?

@dereuromark
Copy link
Member

@dereuromark dereuromark commented May 9, 2015

@sukihub The problem so far was that is was a one-man show with months (> 4 even with critical bugs) of activity break, which is unacceptable for such a widely used library of that magnitude IMO.
We reached out multiple times in the past, offered help and even co-management. To be fair, it got better, but it is still far from ideal. Thus branching off might be a justified solution.

@sukihub
Copy link
Contributor

@sukihub sukihub commented May 9, 2015

@dereuromark Oh, I guess I just never imagined that somebody wouldn't want help with an open-source project, silly me :-) Glad to hear you already tried that and sorry I kinda doubted you :-)

@zoghal
Copy link
Contributor

@zoghal zoghal commented May 10, 2015

👍 good idea

@ionas
Copy link
Contributor

@ionas ionas commented May 12, 2015

The fewer dependencies the better.
If at the same time you CAN decouple things without having to add too much glue, great.

@markstory
Copy link
Member

@markstory markstory commented May 12, 2015

@ionas I wouldn't expect any more glue than we currently have with carbon.

@davidyell
Copy link
Contributor

@davidyell davidyell commented May 28, 2015

As long as it doesn't break BC, I'm 👍

@burzum
Copy link
Contributor

@burzum burzum commented May 28, 2015

👍

@ravage84
Copy link
Member

@ravage84 ravage84 commented May 31, 2015

Sad to see this failing, but 👍

@drmonkeyninja
Copy link

@drmonkeyninja drmonkeyninja commented Jun 2, 2015

👍

@andrecavallari
Copy link

@andrecavallari andrecavallari commented Jun 8, 2015

So... will this standalone lib be created?

Using CakePHP 3.0 with PHP7 I get this error:

Warning (2): Declaration of Carbon\Carbon::createFromFormat() should be compatible with DateTime::createFromFormat($format, $time, DateTimeZone $object = NULL) [ROOT/vendor/nesbot/carbon/src/Carbon/Carbon.php, line 2203]

I fixed it my own hand, but just want to know more about this standalone plugin

@lorenzo
Copy link
Member Author

@lorenzo lorenzo commented Jun 8, 2015

it will, when someone gets the time to do it. Maybe you are interested @andrecavallari ?

@andrecavallari
Copy link

@andrecavallari andrecavallari commented Jun 8, 2015

I already started something here, if it works I will share it

@lorenzo
Copy link
Member Author

@lorenzo lorenzo commented Jun 8, 2015

great!

@lorenzo
Copy link
Member Author

@lorenzo lorenzo commented Jun 14, 2015

@andrecavallari any progress on this?

@andrecavallari
Copy link

@andrecavallari andrecavallari commented Jun 15, 2015

Not yet, still using Carbon ;(

@sdustinh
Copy link
Contributor

@sdustinh sdustinh commented Jun 15, 2015

I haven't looked into this at all (just casually commenting on an issue in my notifications) but if we are kicking around RFC ideas I would love support for the FormHelper datetime format in the API Instantiation so something like this would be easier...

public function validationDefault(Validator $Validator)
{
    return $Validator
        ->add('expires', [
            'in-future' => [
                'rule' => function ($value, $context) {
                    /*
                    $value = [
                        'year' => '2015',
                        'month' => '06',
                        'day' => '15',
                        'hour' => '19',
                        'minute' => '22'
                    ];
                    */

                    return $value->isFuture();
                },
                'on' => function ($context) {
                    return !empty($context['data']['expires']);
                },
                'message' => 'Expiration must be set to a future date.'
            ]
        ]);
}
@wisamhussain
Copy link

@wisamhussain wisamhussain commented Jul 30, 2015

A standalone CakePHP Date/Time library with 0 dependencies would be ideal.

@dereuromark
Copy link
Member

@dereuromark dereuromark commented Aug 19, 2015

With #7263 limitation it is even more important to speed up this process IMO to avoid bugs being production-blockers as we cannot use the newer bugfix versions.

@markstory markstory modified the milestones: 3.1.0, 3.2.0 Aug 26, 2015
@markstory markstory modified the milestones: 3.2.0, 3.1.0 Aug 26, 2015
@lorenzo
Copy link
Member Author

@lorenzo lorenzo commented Nov 22, 2015

@markstory is there anything missing before closing this?

@markstory
Copy link
Member

@markstory markstory commented Nov 22, 2015

Nope.

@markstory markstory closed this Nov 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.