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

[5.5] New Carbon helper #21660

Closed
wants to merge 1 commit into from
Closed

[5.5] New Carbon helper #21660

wants to merge 1 commit into from

Conversation

carusogabriel
Copy link
Contributor

Recently, two new helpers were added to Laravel: now and today. My first reaction was to add another two ones: tomorrow and yesterday, but it didn't sound the best to do. So, encouraged by that, I've created a new helper: carbon.

This helper will make the work with Carbon a lot easier, especially in views, removing the need of importing Carbon every time.

This supports:

  • Datetime: carbon(\DateTime::createFromFormat('Y-m-d H', '2017-10-13 22'));
  • String: carbon('2017-10-13 22:00:00');
  • Timezone: carbon('tomorrow', 'America/Sao_Paulo');
  • Relative phrases from Carbon: carbon('next month');
  • Carbon helpers: carbon()->yesterday().

References: laravel/internals#828

@vlakoff
Copy link
Contributor

vlakoff commented Oct 14, 2017

If $tz = null, the ->setTimezone($tz) would apply the default timezone (see Carbon.php), overriding the timezone possibly already set on the passed DateTime instance.

Therefore, this:

if ($datetime instanceof \DateTime) {
    return Carbon::instance($datetime)->setTimezone($tz);
}

should be replaced with something like:

if ($datetime instanceof \DateTime) {
    $carbon = Carbon::instance($datetime);

    if (!is_null($tz)) {
        $carbon->setTimezone($tz);
    }

    return $carbon;
}

(and adding an unit test as well)

@linaspasv
Copy link
Contributor

linaspasv commented Oct 14, 2017

@Gabriel-Caruso it's very nice PR! I would just suggest to organise the code of your helper function in a similar style like other ones (see: app, config, redirect, etc.)

function carbon($datetime = null, $tz = null)
{
    if (is_null($datetime)) {
        return new Carbon;
    } 
    
    if ($datetime instanceof \DateTime) {
        if (is_null($tz)) {
            return Carbon::instance($datetime);
        }

        return Carbon::instance($datetime)->setTimezone($tz);
    }

    return Carbon::parse($datetime, $tz);
}

@vlakoff
Copy link
Contributor

vlakoff commented Oct 14, 2017

Just a nitpick, I'd replace:

return Carbon::parse($datetime, $tz);

with:

return new Carbon($datetime, $tz);

This saves a function call, and the syntaxic sugar is useless here, as this line is not using fluent syntax.
Also, it emphasize the remaining method, Carbon::instance, which contrarily is justified.

@vlakoff
Copy link
Contributor

vlakoff commented Oct 15, 2017

I had a look at this "carbonize" helper, linked on laravel/ideas#828. There are good things to pick in it.

For instance, this:

if (is_null($datetime)) {
    return new Carbon;
}

should be replaced with:

if (is_null($datetime)) {
    return new Carbon(null, $tz);
}

Test carbon helper

StyleCI

Fix timezone from DateTime override if $tz wasn't passed to carbon func

Fix tests for PHP 7.1 and 7.2 fom Travis CI

Organize conditions to match existing helpers

Use Carbon constructor method instead of parse

Use $tz even if $datetime is null

Create carbon helper
@sisve
Copy link
Contributor

sisve commented Oct 16, 2017

A quick slightly-relevant note; if you actually need working timezone calculations, stay on Carbon 1.21 - briannesbitt/Carbon#863

@taylorotwell
Copy link
Member

No plans to add this to core.

@linaspasv
Copy link
Contributor

linaspasv commented Oct 16, 2017

Omm... so sad. In any case, good job @Gabriel-Caruso , keep the good work coming, this goes to all my new projects as a 3rd party helper!

@furey
Copy link

furey commented Oct 21, 2017

My implementation's been getting a little love on Twitter so thought I'd share…

if (!function_exists('carbon')) {
    /**
     * @param array $params
     * @return \Carbon\Carbon
     * */
    function carbon(...$params)
    {
        if (!$params) {
            return now();
        }

        if ($params[0] instanceof \DateTime) {
            return \Carbon\Carbon::instance($params[0]);
        }

        if (is_numeric($params[0]) && (string) (int) $params[0] === (string) $params[0]) {
            return \Carbon\Carbon::createFromTimestamp(...$params);
        }

        return \Carbon\Carbon::parse(...$params);
    }
}

Pretty much handles all the things, e.g.:

carbon(); // i.e. now()
carbon(1234567890);
carbon("1234567890");
carbon("yesterday");
carbon($myDateTimeInstance);
carbon($myCarbonInstance);

Tipping my hat to @ryanwinchester for the timestamp string conditional.

Goodbye "use Carbon\Carbon"!

🤓👌

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

Successfully merging this pull request may close these issues.

None yet

6 participants