[8.x] Support value callback arguments

[danharrin]

This addition allows the developer to pass arguments to a value() helper callback.

$callback = fn ($arg) => $arg;

value($callback, 'foo'); // `foo`

[GrahamCampbell]

👎 The point of this method is to unwrap thunks. If you have a callable with params, just call it... $value(...$args).

[danharrin]

Hey @GrahamCampbell, I know how to pass params to a callback.

I am looking for the exact functionality that this PR allows to avoid making the type check myself.

[stancl]

@GrahamCampbell

In my code I use this a lot:

/** @var string|callable: string */
public $default;

// ...

if (! $value) {
    $value = is_callable($this->default)
        ? ($this->default)($this, $foo);
        : $this->default;
}

With this PR I could do

value($this->default, $this, $foo);

Which is much more readable. It does the type check for me. I don't need any conditionals, I can just trust that it will run the callback with the given parameters if it's a callback, and if it's not, it just returns the value.

I think that the entire purpose of value() is to avoid type checks and allow for more expressive code. Dan's PR extends this functionality to callbacks with parameters as well without breaking any existing behavior.

If the argument against this is

If you have a callable with params, just call it..

Then you might as well say

If you have a callable, just call it..

And open a PR to remove the helper entirely.

[stancl]

FWIW it'd be nice if the Closure type check were changed to callable — to make passing callables [$like, $this] and new This::class possible too. But that brings some issues:

• The check would have to be is_callable($value) && ! is_string($value) because I think that in the context of this function, 'explode' would be a value, not a callable. $value often comes from user input.
• When $value comes from user input, aren't there other ways this could be misused? The user could make the input contain an array, which would make [$these, $callables] possible, but if I'm not wrong, those would require the first value to be an object, which the user can't forge. So that should be safe, but I'm bringing this up either way.

Happy to hear any feedback about this change. As I said, in my packages and apps I use this pattern a lot, but I also never typehint Closure. It's always callable, since that has the benefit of accepting callable objects too (which are good for complex logic). So if it were up to me, I'd change this to callable as well, but am not 100% sure that it wouldn't bring potential issues.

[GrahamCampbell]

It is important that it is not callable. If the value is a string like 'implode', PHP will think it is callable.

[stancl]

[GrahamCampbell]

Then people using value where they wanted a callable as the value, can't do this anymore.

[GrahamCampbell]

Certainly cannot do this on 8.x.

[taylorotwell]

@GrahamCampbell why can't do it on 8.x? It's a global function - there is no way to override it?

[stancl]

Current implementation:

• string = value
• callable object = value
• callable array = value
• int, ... = value
• Closure = callback

Callable + not string check:

• string = value
• callable object = callback
• callable array = callback
• int, ... = value
• Closure = callback

Bolded options are the only ones that would change.

So the only breaking changes would be where people expected callables to be treated as values, but now they'd be treated as callbacks.

That's a valid point, and there may be cases where this would cause breaking changes. But at the same time I seriously doubt that anyone would be actually affected.

You'd need code like this:

class MyCallback
{
    public function __invoke() {
        return 'foo';
    }
}

$object = new MyCallback;

$closure = function ($args) {
    return new MyCallback;
};

value($object, 'args'); // returns $object
value($closure, 'args'); // executes $closure which returns $object

Which would be extremely ambiguous. One type of callable returns itself and another one gets executed. Plus, very few people knew about value() until recently it seems.

So I agree with @GrahamCampbell that it would technically be a breaking change (sudden unexpected change in behavior, even if you can override it in your own helpers file), but I can't think of a legitimate case where anyone would be affected by this.

A developer who'd be aware of such obscure internal helpers wouldn't write code where one type of callable executes and another type is returned, they'd use a proper type check. So IMO that change would be very low risk.

[rodrigopedra]

Plus, very few people knew about value() until recently it seems.

I can't tell what criteria you consider as "recent", but I use this helper a lot since when I started developing with Laravel (2015). I have seen usage in lots of projects, and many local Laravel developers I talk with use it. I was even introduced to this helper by a peer.

Most common usage I've seen is to allow lazy evaluation of parameters on a service class' method.

Certainly cannot do this on 8.x.

Agree with @GrahamCampbell on this, one project I worked with and migrated to Laravel 8 just at the end of last year would certainly break.

That project had a custom rule-based engine and accepted tuples (array callables), among other values, that should be evaluated later.

Don't blame me, I was just hired to add a feature on that project. Shared just to point I've seen a project that would break with this change.

even if you can override it in your own helpers file

@stancl if you can share how to do it I would really appreciate. I needed this in the past and failed as it seemed to me composer autoloaded files from dependencies before the project's ones.

I even just tried it again and got this error:

> Illuminate\Foundation\ComposerScripts::postAutoloadDump
PHP Fatal error:  Cannot redeclare value() (previously declared in /home/rodrigo/code/test/vendor/laravel/framework/src/Illuminate/Collections/helpers.php:182)

I added a "files": [...] entry to my composer.json file as I thought that would be the way to do it.

[stancl]

Agree with @GrahamCampbell on this, one project I worked with and migrated to Laravel 8 just at the end of last year would certainly break.

That project had a custom rule-based engine and accepted tuples (array callables), among other values, that should be evaluated later.

What exactly would break? Were you calling value() on those tuples? And had Closures returning the tuples?

[rodrigopedra]

Well I can't share the exact code as it is from a client.

But I will try to describe it.

There are many Criteria classes, and many Effect classes. An Action is a class that have a collection of Criteria and a collection ofEffect

Both Criteria and Effect have a canApply() method, and Effect classes have a apply() method.

If a model, passes all Criteria's canApply checks, and all Effect's canApply methods (let's say an action only has the effect of soft-deleting the model, but it is already soft-deleted, so that action would not be available to it), it is eligible to that action.

When applying an action, it iterates through each Effect and call their apply method, which return a callable (\Closure or non-closure). They are evaluated with value(), so \Closure ones are applied while iterating, and the remaining callables are collected to be applied later, because some can depend on the results of other effects (for example an effect of sending an email, can depend on an effect that changes the assignee of a task model).

Also, those collected "tuple" callables are later called with the container (app()->call($effect)), as some need dependencies to be injected.

Reason they can be out-of-order is because that project had a front-end that allowed users to build custom actions. Criteria and Effects are pre-defined, but they can have arguments ("Task status is one of these: ..." ).

I would do some stuff differently, but as I said I was just hired to add a feature and help migrate to Laravel 8, but again I just wanted to share a case I've seen recently that would break with the discussed change.

Sorry if description is not clear enough, I am working on improving my English skills. But I hope you could see the use case.

Also @stancl , if you can share how to override a global helper I would really appreciate, sorry if it sounded I was being ironic or something like that, if that is why you didn't answer it wasn't my intention, I am not a native speaker. But that is something I really needed in a previous project (migrating a legacy project to Laravel) and couldn't find a way to do so.

If you think this is not the best place to share it, my twitter handle is the same as my github's. Thanks in advance.

[rodrigopedra]

One more thing: I am not opposed on the change per se. I actually find it very useful and more intuitive on usage.

Also I am not talking about the changes from this PR, but from that future changes discussed about accepting callables other than \Closure.

I am just saying I agree it is a breaking change on a minor/patch release. As some developers relying on the old behavior would be surprised with the change.

In the usage I described, they can very easily add a local helper to hold the current behavior.