Skip to content

Commit

Permalink
remove unnecessary foreach from is() method (#24872)
Browse files Browse the repository at this point in the history
  • Loading branch information
@fmwww @taylorotwell
fmwww authored and taylorotwell committed Jul 17, 2018
1 parent 23f6279 commit 57d7b1f
Showing 1 changed file with 1 addition and 7 deletions.
8 changes: 1 addition & 7 deletions src/Illuminate/Http/Request.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,13 +184,7 @@ public function segments()
*/
public function is(...$patterns)
{
foreach ($patterns as $pattern) {
if (Str::is($pattern, $this->decodedPath())) {
return true;
}
}

return false;
return Str::is($patterns, $this->decodedPath());
}

/**
Expand Down

10 comments on commit 57d7b1f

@ortegon000
Copy link

Choose a reason for hiding this comment

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

this is causing aun issue
Will this stay like this?
issue: preg_quote() expects parameter 1 to be string, array given

@browner12
Copy link
Contributor

Choose a reason for hiding this comment

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

what are you providing as input to the ...$patterns parameter?

@rodrigopedra
Copy link
Contributor

Choose a reason for hiding this comment

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

I had a production issue with it also. Previously I passed an array as a parameter, after the update I got the same error as @ortegon000 reported. Luckly I used it only in one place of the project so it was a minor outage.

// previously
if ($request->is( [ 'pathA', 'pathB' ] )) { /* ... */ } // crashes after this update

// currently
if ($request->is( 'pathA', 'pathB' )) { /* ... */ } // remove the array brackets in the argument list

@rodrigopedra
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is a regression as in many places in the framework you can opt to use an array or rely on the spread operator, such as middleware only/expcet options, array helper funcions (eg. array_only), request only/except, and many other places.

I prefer to use arrays when passing this multiple arguments, just for personal preference, I can adjust my mindset, but maybe we need a test to avoid breaking apps with this kind of updates

@browner12
Copy link
Contributor

Choose a reason for hiding this comment

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

you code should not be affected by this change, because we normalize everything to an array when it comes in. I'm digging into this right now

@rodrigopedra
Copy link
Contributor

@rodrigopedra rodrigopedra commented on 57d7b1f Jul 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The splat in the argument list wraps the array inside another array.

$a = function ( ...$args ) { dd( $args ); };

$a( [ 1, 2, 3 ] ); // dumps [ [ 1, 2, 3 ] ]

So previously if passing an array the foreach iterated through the outer array generated by the splat operator on argument list and passed the inner array to the Str::is(...) which handles array arguments well.

Without the foreach, the functions passes this double-array (outer and inner array) and then the error occurs, as the Str::is iterates through the outer array and tries to handle the inner array as a string.

It was an easy fix in my app, but maybe we should include a note on the docblock to not pass an array. The weirdst thing is that the routeIs method which is just below the is method in the Request class and does not use the foreach previously used in the is method. By git blaming it, it was added much later and maybe is not so well know as its counterpart.

@Miguel-Serejo
Copy link

Choose a reason for hiding this comment

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

The docblock already lists the parameter as dynamic, which is explicitly different from array|string which is what your usage seems to suggest you were using it as.

@browner12
Copy link
Contributor

Choose a reason for hiding this comment

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

absolutely right. I had to learn how the splat actually passes through.

here is the only documentation I could find for the method:

https://laravel.com/docs/5.6/requests#request-path-and-method

It is a little lacking, and doesn't even discuss checking multiple paths.

While going forward I would say it should be used dynamically, I would say this is a breaking change that should not have been merged in a point release.

@philmareu
Copy link

Choose a reason for hiding this comment

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

I also am having an issue with this change. I'm passing in an array as I always have been and now it is throwing the error as mentioned before. It is adding my array inside another array. So it is now iterating through and trying to preg_quote an array.

@rodrigopedra
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @philmareu , @browner12 already sent PR #24885 that reverts the change and it got merged. But it is not released yet. You can rollback to 5.6.27 while waiting for the next release to be tagged.

Please sign in to comment.