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] Add in() and inHidden() functions to Context Stacks #52346

Merged

Conversation

lessevv
Copy link
Contributor

@lessevv lessevv commented Jul 31, 2024

I'm really liking the addition of Context and the support for stacks. But I was missing a function where you could easily check if a certain value actually is within a stack. It's basically just adding in_array() support for the stacks.

You could use it as follows:

# Normal
Context::push('foo', 'bar', 'lorem');

Context::in('foo', 'bar'); # true
Context::in('foo', 'doesNotExist'); # false

# Hidden
Context::pushHidden('foo', 'bar', 'lorem');

Context::inHidden('foo', 'bar'); # true
Context::inHidden('foo', 'doesNotExist'); # false

@KaniRobinson
Copy link

This is really cool! Very handy for when you're building analytics around contextual requests, or pulling out trends.

@lessevv lessevv changed the title [11.x] Add in() and inHidden() functions to Context Stacks [11.x] Add in() and inHidden() functions to Context Stacks Jul 31, 2024
@lessevv
Copy link
Contributor Author

lessevv commented Jul 31, 2024

A thought that just passed my mind, I could possibly also make the $strict an optional passable parameter for the in_array. Where it's non-strict by default, but can be made strict by passing true.

Not sure if that's needed, though.

@KaniRobinson
Copy link

An optional parameter isn't going to be breaking, I'd go for it.

This would be super useful in pulse!

@lessevv
Copy link
Contributor Author

lessevv commented Jul 31, 2024

Added it, and a test to ensure strictness works!

*
* @throws \RuntimeException
*/
public function in(string $key, mixed $value, bool $strict = false): bool
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if in is too generic. Feels like you are checking that the key is in the context.

Wonder if Context::stackContains might be better.

That also has symmetry with the Collection::contains method. It could accept a closure so that you can do more complex checks if needed.

Context::stackContains($stack, $value);

Context::stackContains($stack, function ($value) {
    // ...
});

That way we don't need the strict flag either, as the closure gives you that escape hatch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would indeed be more inclined to go this way if the underlying stack was a collection, but it seems to be a deliberate choice to have this as an array. As the rest of the functions, such as has() do an array_key_exists(), I would say the current implementation is more true to the current implementation of Context in general.

Not necessarily shooting this down, but I feel this is more something for down the line if (if ever) Context receives a bit of a bigger update.

@taylorotwell taylorotwell merged commit 4980576 into laravel:11.x Aug 1, 2024
1 check passed
@timacdonald
Copy link
Member

I've proposed a change to the closure based functionality in #52381

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.

4 participants