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] Allow when() to accept Closure condition parameter #54003

Closed
wants to merge 1 commit into from

Conversation

ziadoz
Copy link
Contributor

@ziadoz ziadoz commented Dec 23, 2024

This PR allows the standalone when() helper function to accept a Closure as the condition, which brings it into line with the Conditionable trait, which behaves the same way. It's handy when the condition may be expensive or a few lines of code.

I also did notice the helper method here is in the Collections namespace, however the tests suggest it belongs in Support. However I figured this might be a breaking change so I left it where it is, but I can move it if necessary.

@@ -248,6 +248,8 @@ function value($value, ...$args)
*/
function when($condition, $value, $default = null)
{
$condition = $condition instanceof Closure ? $condition() : $condition;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$condition = $condition instanceof Closure ? $condition() : $condition;
$condition = value($condition);

@devajmeireles
Copy link
Contributor

Good catch! I sent a Laravel-way suggestion.

@zakariaarrid
Copy link

Hi @ziadoz ,
You can update the PHPdoc.
* @param \Closure|mixed $condition

@taylorotwell
Copy link
Member

Technically a breaking change, though likely rare.

@shaedrich
Copy link
Contributor

I wonder what the value/benefit is in executing a callable within the function but not passing any contextual variables to it that you wouldn't necessarily have outside of the function. In Conditionable, $this is passed to the callback.

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.

5 participants