[5.6] revert #24872

[browner12]

#24872 causes a BC break when the user passes an array to the function.

$request->is(['pathA', 'pathB']);

This code worked on previous versions, but not after the change due to the fact the method uses the splat operator which wraps the input array in an additional array.

While I agree it should not be used this way in the future, and should be changed in 5.7, I believe it is a BC break for 5.6. If this is accepted, I will make the PR to 5.7.

[antonkomarev]

It would be great to have this covered with tests to prevent future issues.

[JackWH]

Thanks for fixing this, it had me stumped for a while!

What is the justification for not using it this way in future? I understand it's not documented to accept an array, but it's extremely useful being able to match an is() check against multiple entries at once.

For example, I'm using it to add UI active classes to menu items that could potentially be highlighted as active against several different URLs.

Just throwing it out there, as it would be a shame to see that functionality removed.

[Miguel-Serejo]

You can still pass multiple values into the function, just as a dynamic parameter instead of an array, like this:

$request->is('route_a', 'route_b', 'route_c');

[browner12]

@JackWH you can still check multiple URLs at once, you just need to adjust what you pass a little.

Instead of:

$request->is(['pathA', 'pathB']);

you would use

$request->is('pathA', 'pathB');

This is because the function uses a variadic input, and will turn your inputs into an array.

[JackWH]

Thanks both, that helps! Cheers.

[Miguel-Serejo]

An alternative solution would be to change the parameter from dynamic to array|string to allow the current (apparently) expected usage instead.

Or, because the docblock technically doesn't tell you what type of input it expects, other than the fact it can handle many, we could interpret that as allowing dynamic(array|string) (unsure of the syntax for declaring that) and just leaving the method as-is.

[JackWH]

I can definitely see an advantage in allowing arrays here. In my use case, my Blade template looks like this:

@foreach($menuItems as $menuItem)
    <li class="menu @if(Request::is($menuItem->matchingUrls)) active @endif"> ... </li>
@endforeach

The $menuItem->matchingUrls property is an array defined within a View Composer, where I'm handling some logic as well. I'm not sure how I'd set up that object with a list of URLs, then pass those over as variadic parameters to the Request::is() call.

I'm sure I can find a way around it, but this is just one example of where accepting arrays would be useful (unless I'm missing something!).

[browner12]

What if we added a conditional to normalize it?

$patterns = (is_array($patterns[0]) ? $patterns[0] : $patterns;

this could probably even go to 5.6 still and allow us to remove the extra foreach

[JackWH]

That seems good. For consistency, I think we'd also want to add the same normalisation to routeIs() and fullUrlIs().

I'm happy to make a pull request (gotta get that sweet Contributor badge!), presuming nobody else has started already...

[RoccoHoward]

Just change your code so that you splat your array:- @foreach($menuItems as $menuItem)<li class="menu @if(Request::is(...$menuItem->matchingUrls)) active @endif"> ... </li>@Endforeach

…

On Fri, 20 Jul 2018 at 00:43, Jack W-H ***@***.***> wrote: I can definitely see an advantage in allowing arrays here. In my use case, my Blade template looks like this: @foreach($menuItems as $menuItem) <li class="menu @if(Request::is($menuItem->matchingUrls)) active @endif"> ... </li> @Endforeach The $menuItem->matchingUrls property is an array defined within a View Composer, where I'm handling some logic as well. I'm not sure how I'd set up that object with a list of URLs, then pass those over as variadic parameters to the Request::is() call. I'm sure I can find a way around it, but this is just one example of where accepting arrays would be useful (unless I'm missing something!). — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#24885 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN6OACM0fdMEOmEa_ILpM85w-Id1NVdks5uIJsCgaJpZM4VVCmF> .

[JackWH]

@RoccoHoward Aha! Didn't know you could make the function call like that (thought it was just a definitional thing). Cheers!

[browner12]

I don't think routeIs behaves the same way, but updating fullUrlIs is probably a good idea as well.

definitely adding tests for this as well.

[Miguel-Serejo]

That dynamic docblock has been bugging me for a while. I can't find any documentation on that keyword anywhere.
I did, however, find references to the proper syntax for variadic parameters scattered about, notably missing from the official phpdoc documentation: @param string ...$param
I'd like to propose changing the docblock to avoid future confusion, as dynamic is too vague and doesn't actually tell us anything regarding the expected parameters.

Now the question is, what do we want to support here? Currently, it works with two types of input: variadic strings (->is('one', 'two', ...)) and variadic arrays (->is(['one', 'two'], ['three', 'four'])). These are supported because of the double iteration.

If we want to support the current possible usages, we simply need to change the docblock to @param array|string ...$patterns. This could be considered a documentation bug fix.

If we want to support arrays, but not splats, we need to change this from accepting ...$patterns to just $patterns and change the docblock to @param array|string $patterns (or the more explicit but less common string[]|string and we can pass the input straight to Str::is. This would be a breaking change.

If we want to support string splats, but not arrays, keep the current signature, change the doblock to @param string ...$patterns and change the implementation, also a breaking change.

Sources:
variadic param syntax:

• https://stackoverflow.com/questions/13787426/how-to-declare-unlimited-variadic-parameters-in-docblock#24336702
• Variable arguments do not work phpDocumentor/phpDocumentor#629

string[] array syntax:
https://docs.phpdoc.org/references/phpdoc/types.html#arrays

[browner12]

If we work backwards, and look at how we want to consume it, I would prefer to support sending either multiple comma separated strings (how it is currently documented)

$request->is('pathA', 'pathB', 'pathC');

or a single array

$request->is(['pathA', 'pathB', 'pathC']);

I would imagine the final option of multiple arrays is very uncommon if even used at all

$request->is(['pathA', 'pathB', 'pathC'], ['pathD', 'pathE']);

This is consistent with other parts of the framework where we can either send a string or an array, and have it normalized on the backend.

To implement this usage, we would need to implement the normalization I proposed above:

$patterns = is_array($patterns[0]) ? $patterns[0] : $patterns;

and unfortunately there is no docblock to accurately describe this scenario.

Thoughts? Is this how people would prefer to consume it? If we continue supporting all 3 options above, we cannot perform the proposed optimization of removing the foreach, although to be fair, in some Blackfire tests I ran, the results did not show a significant difference in the two variations.

Also, random note I learned as i was looking into this, you can actually type hint the variadic parameter!

public function is(string ...$patterns) {
}

[Miguel-Serejo]

Due to the docblock constraint, I would be more inclined to just maintaining the current implementation and behavior. The cost of a foreach is basically nothing (to the point where I'm fairly sure there's a function in the framework that does a foreach($array as $item) return $item; instead of return $array[0] because it's faster) so the net gain from the refactor may just not be worth subtracting functionality, documented or not.

[browner12]

agreed. now that I've seen there's no real performance gain, I'm less inclined to push this through.

would've been nice to turn that function into a one-line, though....

[Miguel-Serejo]

Random thought, what if Route::is becomes what it is documented to be, accepting a single string, and a new Route::isAny method takes on the current behavior?

Would that be worth the hassle at all?

[browner12]

I would say not worth the hassle, I don't see any clear benefit from that.

[Miguel-Serejo]

You'd get your one-liner. That's about it.

So, unless I'm misreading the whole situation here, all that's left to do is update the docblock and possibly the documentation?

[browner12]

I have a feeling you're going to have a tough time pushing through this one off docblock update, since the dynamic keyword appears 15 total times in the FW. Would it be more appropriate to do an all encompassing PR to update the docblocks to the @param string ...$parameter syntax?

[Miguel-Serejo]

That would be preferable, but a cursory search revealed inconsistent usage of dynamic. I'll take a closer look at it tomorrow.