Ensures that 'optional' method returns instance of Optional

[me-shaon]

It is expected that the optional helper method would return an instance of Illuminate\Support\Optional.
Currently, the following code would throw an exception:

optional(null, function ($value) {
    return $value;
})->invalidProperty;

So, I've modified the code a bit to fix it. This PR is to ensure that the callback method would only be called for not null $value.

[jmarcher]

Do not change existing tests add new ones.

[me-shaon]

@jmarcher what if the existing test was wrong?
This PR is actually changing the behavior of a method, which wouldn't pass for the existing Test. That's why I had to change it. What's the good approach to do it?

[jmarcher]

The exception itself says what Optional is supposed to do: The optional callback should not be called for null.

[me-shaon]

That's actually what the code is doing, the optional callback is not called for null. But would something like the following be more appropriate as the Test?

$this->assertNull(optional(null, function () {
            return '1';
        })->missing);

[Miguel-Serejo]

The existing test is there to verify that the callback isn't called for null values. Please do not change it.

The problem you're encountering is due to optional() returning void when you pass it a null value and a non-null callback. Add a test to verify that your changes fix that, but don't change any existing tests so it's clear you're not breaking anything.

[me-shaon]

@36864 yup. But If I keep the Test then my change would return an instance of Optional class for a null value and a non-null callback, and the test would fail because it is expecting a null. What should I do about it?

[jmarcher]

If that is the case it means it is a breaking change Ahmed shamim <notifications@github.com> schrieb am Mi., 29. Aug. 2018, 11:31:

…

@36864 <https://github.com/36864> yup. But If I keep the Test then my change would return an instance of Optional class for a null value and a non-null callback, and the test would fail. What should I do about it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#25370 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAp6-9vjGQNKkYFULKuh4X37X_zaLk4wks5uVl9wgaJpZM4WRFFy> .

[me-shaon]

@jmarcher I guess so. What to do about it then? I'm not that experienced and need suggestions. :)

[jmarcher]

First of all target 5.7 (Unreleased) and give a more extensive description of why this change is needed.

[taylorotwell]

Don't change existing tests.

[me-shaon]

@taylorotwell I think this test case isn't right. Whether or not I provide a callback, if the $value is null, optional method should return an instance of Illuminate\Support\Optional Class, not a null. If it returns a null, then the purpose of using optional is useless. Or am I not getting it right?

[AkenRoberts]

After talking with @me-shaon on Slack #internals a bit, we both found it odd that optional() is essentially doing two completely different tasks depending on the existence of the callback. That's basically the issue here. Consider the documentation:

The optional function accepts any argument and allows you to access properties or call methods on that object. If the given object is null, properties and methods will return null instead of causing an error

Great, that's how it has always worked.

The optional function also accepts a Closure as its second argument. The Closure will be invoked if the value provided as the first argument is not null

Providing a Closure effectively negates any optional logic that would be there without the callback. If a user tries to combine the two, they're going to run into bugs and wonder why optional() isn't doing what it said it should be.

I'm still fuzzy on how this functionality is helpful, also. Genuinely curious about the use cases where this is really handy, since I can't think of any.

At minimum, the documentation should be improved to avoid confusion.

[Miguel-Serejo]

This was added in #23688, and the use case was explained.
I agree that it's odd that optional serves two completely different purposes, but changing it now would be a breaking change.

Maybe the example usage from the PR could be added to the docs?

[AkenRoberts]

Thank you @36864. @JosephSilber did join us on Slack and also shared a good real-world use case that helped explain it for me.

I can see more clearly now how this functionality is useful. I still don't agree that it was overloaded with optional(), however that's how it is now. So updating the documentation to make that clear should be sufficient. 👍