[11.x] revert #52510 which added a unneeded function call

[rodrigopedra]

This PR reverts PR #52510

PR #52510 introduced a call to the value() helper on the transform() helper's last return.

The value() helper only checks if the $value argument is a \Closure, and if so, invokes it. Otherwise, it returns the plain $value without any changes:

framework/src/Illuminate/Collections/helpers.php

Lines 234 to 237 in 81a0b5b

	function value($value, ...$args)
	{
	return $value instanceof Closure ? $value(...$args) : $value;
	}

The transform() helper already checks if the $default value is a callable (which includes the \Closure case), before the modified line:

framework/src/Illuminate/Support/helpers.php

Lines 468 to 479 in 81a0b5b

	function transform($value, callable $callback, $default = null)
	{
	if (filled($value)) {
	return $callback($value);
	}
	if (is_callable($default)) {
	return $default($value);
	}
	return value($default);
	}

Therefore, when execution reaches the modified line on PR #52510 we already know $default is not a callable, and thus, not a \Closure.

Which renders the modification unneeded, as it only adds a useless function call, and a ternary check, that will always evaluate to false and return the unmodified value the value() helper receives.

This PR:

• Reverts the line modified by PR [11.x] Applying value Function into the $default value of transform helper #52510 to remove the unneeded value() call.

[rodrigopedra]

As a disclaimer, I first commented on PR #52510, just checking what changed, and without checking the transform() helper full implementation. And it seemed to me a good idea at the time.

#52510 (comment)

After revisiting it, I noticed it was unneeded.

[devajmeireles]

value is widely used and only aims to interpret the value associated with $default. There are no breaking changes. I think you need to get some sleep 😂 Taylor has been dealing with this for over 10 years, he wouldn't accept it if he didn't see the point, which in this case is about standardization.

[rodrigopedra]

I won't tell you what you need to do, it seems you are an adult already, so you should know best how to behave in public spaces, specially in an environment where people are trying to collaborate in their spare time.

But I'd advise you to read Laravel's code of conduct:

https://laravel.com/docs/11.x/contributions#code-of-conduct

And by the way, I've been helping to improve the framework for almost 9 years. Maybe I have learned a thing or two along the way.

https://github.com/laravel/framework/pulls?q=is%3Apr+is%3Amerged+author%3Arodrigopedra

You are more than welcome to hold any opinion you want, as long as you keep it respectful when sharing it publicly.

[crynobone]

I tend to agree with @rodrigopedra that the original PR doesn't provide anything that the original code already done. But @devajmeireles you should be able to provide a tests example showing where value() should be useful in transform()

[rodrigopedra]

An alternative would be simplifying transform()'s implementation to:

function transform($value, callable $callback, $default = null)
{
    if (filled($value)) {
        return $callback($value);
    }

    return value($default, $value);
}

But that is technically a breaking change, as current transform() accepts a callable as its $default, whilst value() only checks for a \Closure.

Maybe for 12.x we could consider the value() helper to use is_callable() instead of checking for a \Closure?

[donnysim]

@rodrigopedra callable would be dangerous on value as a value('app') would return Application and many other string values including classes would result in a callable and end up in unexpected results.

[crynobone]

@donnysim value() helper only accept Closure and not callable.

[donnysim]

@crynobone yeah yeah, should've quoted what I was responding to, my bad. It's for the

Maybe for 12.x we could consider the value() helper to use is_callable() instead of checking for a \Closure?

[rodrigopedra]

@donnysim you are right, thanks for the heads up!