-
-
Notifications
You must be signed in to change notification settings - Fork 151
PHP 5.6: Use variadics and argument unpacking #80
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not so simple because it changes signature. Previously the signature suggested that findPrefix(['bar', 'baz']) is preferred. Now it suggests that findPrefix('bar', 'baz') is preferred. Do we really want to prefer the variadic syntax over normal array? Passing directly array is slightly faster on PHP 7. AFAIK we should pick one syntax and deprecate the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is true, also the name implies one string argument, not an array. In PHP 7 we should pick one and stick with it (array $strings vs string ...$strings).
This is a common scenario through Nette though, e.g. nette/finder#4 is similar.
Either way this is a good thing to think about - do we want to prefer variadics & unpacking or untyped arrays?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vote for variadic. Anyone can always unpack array in function call when needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vote for variadic because of better type hint string ...$strings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO scenario if (!is_array($var)) $var = func_get_args() can be deprecated when Nette will require >= 5.6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dg Do the signature would be changed to array $strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is used very rarely, but common usecase is probably findPrefix($a, $b), so ...$strings fits better and posibility to add PHP7 typehint string ...$strings sound good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 The possibility to typehint the value with variadics is good. And the condition above is there just for BC now (it won't make sense anyway once the method gets string typehint).
Passing directly array is slightly faster on PHP 7.
That makes sense, but is the difference noticeable at runtime?
93e44ad to
de33c94
Compare
PHP 5.6: Use variadics and argument unpacking
Depends on #79 (for composer & Travis PHP version bump).