[5.6] Use Carbon 1.26

[kylekatarnls]

Now that Carbon\Carbon supports macros and JSON, Illuminate\Support\Carbon should be emptied, then it would be the end of compatibility issues.

[Miguel-Serejo]

At a minimum this needs to require "^1.26.2" due to the problems with 1.26/1.26.1.

This is also probably a breaking change for anyone extending Support\Carbon into their own class and should target 5.7

[kylekatarnls]

Nope, because by emptying the Illuminate\Support\Carbon content (no longer needed), there is no conflict anymore.

And this should also be done in master for 5.7.

But we need a last alignment on the macro feature (to work both with PHP 5.3, our Carbon minimal requirement and with the $this context passing that is not available on PHP 5.3). I'm working on it.

[tillkruss]

This is a breaking change. Either send it to v5.7, or don't remove Macroable.

[kylekatarnls]

No, it's not since compatible methods are now in Carbon itself.

[tillkruss]

You're saying that peoples existing macros won't break with this change?

[devcircus]

Throwing in my vote to keep Illuminate\Support\Carbon as-is. Would love for Laravel to maintain control over Carbon macros/mixins and ensure the functionality stays in sync with the rest of Laravel's macros/mixins.

I understand that it makes sense to relinquish control to the library itself, just my opinion.

Further, I'd like to see Laravel move away from Carbon completely and adapt an immutable library

[tillkruss]

You're saying that peoples existing macros won't break with this change?

[arjasco]

Firstly I thank @kylekatarnls for your hard work bring Carbon back to life.

As far as I'm aware the only reason Carbon was subclassed in Laravel is to support macros (see here #19771) It's quite an odd decision really that this was accepted in my opinion..

Carbon is a package in its own right, it shouldn't have to consider that the same macroable feature exists outside of it. It makes sense to me to have macros within Carbon because of the vast possible time comparisons one might need it's already a huge file to meet most use cases.

Maybe this is a decision to remove Illuminate\Support\Carbon altogether from 5.7

[devcircus]

It should not break existing apps. Lucas tried to push this functionality to Carbon before sending to Laravel. It was rejected. Now Carbon maintainers have changed their minds and copied the functionality over to their package.

The macro and mixin methods will continue to work, however I prefer keeping it in Illuminate\Support.

[kylekatarnls]

@devcircus Indeed, I forgot that.

@tillkruss Our macro methods macro, mixin and hasMacro are the same as in Macroable.php

__callStatic is strictly equivalent:

Macroable:

public static function __callStatic($method, $parameters)
{
    if (! static::hasMacro($method)) {
        throw new BadMethodCallException(sprintf(
            'Method %s::%s does not exist.', static::class, $method
        ));
    }
    if (static::$macros[$method] instanceof Closure) {
        return call_user_func_array(Closure::bind(static::$macros[$method], null, static::class), $parameters);
    }
    return call_user_func_array(static::$macros[$method], $parameters);
}

Carbon:

public static function __callStatic($method, $parameters)
{
    if (!static::hasMacro($method)) {
        throw new \BadMethodCallException("Method {$method} does not exist.");
    }

    if (static::$localMacros[$method] instanceof Closure && method_exists('Closure', 'bind')) {
        return call_user_func_array(Closure::bind(static::$localMacros[$method], null, get_called_class()), $parameters);
    }

    return call_user_func_array(static::$localMacros[$method], $parameters);
}

For the magic __call, the only difference is the support of PHP 5.3 and 5.4, we pass $this as $self is the last argument for the closure is $self. So except if someone used $self for the last argument without referring the current instance, all the existing macros will continue to work.

Macroable:

public function __call($method, $parameters)
{
    if (! static::hasMacro($method)) {
        throw new BadMethodCallException(sprintf(
            'Method %s::%s does not exist.', static::class, $method
        ));
    }
    $macro = static::$macros[$method];
    if ($macro instanceof Closure) {
        return call_user_func_array($macro->bindTo($this, static::class), $parameters);
    }
    return call_user_func_array($macro, $parameters);
}

Carbon:

public function __call($method, $parameters)
{
    if (!static::hasMacro($method)) {
        throw new \BadMethodCallException("Method {$method} does not exist.");
    }

    $macro = static::$localMacros[$method];

    $reflexion = new \ReflectionFunction($macro);
    $reflectionParameters = $reflexion->getParameters();
    $parametersCount = count($reflectionParameters);
    if ($parametersCount > count($parameters) && $reflectionParameters[$parametersCount - 1]->name === 'self') {
        $parameters[] = $this;
    }

    if ($macro instanceof Closure && method_exists($macro, 'bindTo')) {
        return call_user_func_array($macro->bindTo($this, get_class($this)), $parameters);
    }

    return call_user_func_array($macro, $parameters);
}

I can understand the willing of keeping Illuminate\Support\Carbon as is. Just be aware this duplicate Carbon 1.26 methods.

About jsonSerialize, this part is strictly identical.

I can add I take the engagement not to change anything in the methods moved from Illuminate\Support\Carbon.

[Miguel-Serejo]

This is still a breaking change. Any code relying explicitly on the presence of the Macroable trait in the class will break.

>>> class_uses(Carbon\Carbon::class);
=> []
>>> class_uses(Illuminate\Support\Carbon::class);
=> [
     "Illuminate\Support\Traits\Macroable" => "Illuminate\Support\Traits\Macroable",
   ]

[kylekatarnls]

@36864 Seems very specific, but indeed. This argument is sufficient to me for waiting for a minor release (such as 5.7).

I know it's easy to say now that it's done, but such ability should be tested with interfaces rather than traits. Maybe on the long term, a MacroableInterface should be added (maybe submitted to https://www.php-fig.org/psr/) with macro, mixin, hasMacro. And this interface should be used to test if macro stuff is available rather than testing the trait has been used. Don't you think so?

[taylorotwell]

No plans to make this change on 5.6.

[kylekatarnls]

Legit, but does #23896 can be reconsidered so?

And, what about a PR for the JSON part?

[X-Coder264]

@kylekatarnls I think you should resubmit #23896 because Taylor usually doesn't come back to check for new comments in the PRs which are closed (such as this one).

[SolStis86]

https://soundcloud.com/hardcore-messiah/dougal-b2b-gammer-slammin-vinyl-nye-2007 1:13:00 -> exciting. Beyond this... WHY is it happening. There is no explanation as to the failing tests.