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

[11.x] Applying value Function into the $default value of transform helper #52510

Merged

Conversation

devajmeireles
Copy link
Contributor

The $default value of the transform function does not pass through the value to allow the execution of a closure to obtain the default value, for example. This PR fixes this by passing $default through the value helper.

@taylorotwell
Copy link
Member

Would this be considered a breaking change?

@devajmeireles
Copy link
Contributor Author

Would this be considered a breaking change?

I strongly believe not, since both value and transform set $default in the same way.

@rodrigopedra
Copy link
Contributor

rodrigopedra commented Aug 18, 2024

Only if someone is expecting the $default to be a callable, then instead of getting it, they would get the result of its execution.

Which, IMO, is highly unlikely.

@taylorotwell taylorotwell merged commit 29e8040 into laravel:11.x Aug 19, 2024
31 checks passed
@rodrigopedra
Copy link
Contributor

@taylorotwell actually this is unneeded, re-reading my comment, I fell I oversaw it. The transform helper already checks if the $default is a callable, and if so calls it with the $value as a parameter:

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

The line modified in this PR only adds a function call to the stack call, as $default never be a callable at this point:

// $default is never a callable here
return value($default);

Other than the useless function call, there is no harm in keeping it, so I didn't send a PR to revert it.

@devajmeireles
Copy link
Contributor Author

@taylorotwell actually this is unneeded, re-reading my comment, I fell I oversaw it. The transform helper already checks if the $default is a callable, and if so calls it with the $value as a parameter:

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

The line modified in this PR only adds a function call to the stack call, as $default never be a callable at this point:

// $default is never a callable here
return value($default);

Other than the useless function call, there is no harm in keeping it, so I didn't send a PR to revert it.

We're not just talking about callables.

@rodrigopedra
Copy link
Contributor

Just to be clear, you did check the value() implementation, right?

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

If the $value argument isn't a \Closure, this helper just returns the $value variable.

As the if clause just before the modified line excludes the possibility of $value being a callable, the line modified in this PR adds no value, and worst, just adds a function call.

We're not just talking about callables.

Now you let me curious, I guess we mutually agree that in the modified line, $value will never be a callable, and thus never be a \Closure, which excludes the \Closure path from the value() ternary implementation, right?

So, what are you talking about, then?

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