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

[8.x] Use "Conditionable" in existing classes that implement when() #37561

Merged
merged 4 commits into from
Jun 1, 2021

Conversation

inxilpro
Copy link
Contributor

@inxilpro inxilpro commented Jun 1, 2021

Right now, BuildsQueries, Mailable, MailMessage, and Stringable all implement their own version of when() that is exactly the same as the implementation in the new Conditionable trait. This PR just swaps them out for the new trait instead.

(This means that Mailable gets an unless() method as well. All other classes had both a when() and unless() method already.)

It's also worth noting that unless() was implemented in two different ways in the framework. One implementation was a mirror of the when() implementation, while another approach was to call $this->when(! $value, ...). It turns out that the latter solution (calling when()) is incorrect, because it ends up passing the result of ! $value to the callback, rather than the actual value. This PR fixes that issue in the Conditionable trait, thus standardizing it across all the classes that use it.

@bonroyage
Copy link
Contributor

@inxilpro you can also add it to Illuminate\Support\Traits\EnumeratesValues while still maintaining the higher order functionality.

    use Conditionable {
        Conditionable::when as conditionableWhen;
    }

    public function when($value, callable $callback = null, callable $default = null)
    {
        if (! $callback) {
            return new HigherOrderWhenProxy($this, $value);
        }

        return $this->conditionableWhen($value, $callback, $default);
    }

@taylorotwell
Copy link
Member

taylorotwell commented Jun 1, 2021

Is there any change in behavior at all? Also, please keep traits in alphabetical order like the rest of the framework.

@inxilpro
Copy link
Contributor Author

inxilpro commented Jun 1, 2021

Is there any change in behavior at all?

The only change in behavior is technically a bug fix:

Str::of('foo')->unless(null, fn($string, $value) => assert(is_null($value)));

As of Laravel 8.5.19, this will trigger a warning, because the Stringable implementation of unless() will pass a true to the callback rather than the original null. After this change, the above will work.

I'll add a test to demonstrate.

@inxilpro
Copy link
Contributor Author

inxilpro commented Jun 1, 2021

@inxilpro you can also add it to Illuminate\Support\Traits\EnumeratesValues

I thought about that, but honestly, it feels much clearer to just leave it as-is.

@taylorotwell taylorotwell merged commit 93019f1 into laravel:8.x Jun 1, 2021
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.

3 participants