Skip to content

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.

Sign up for GitHub

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

Merged
dg merged 1 commit into from
Aug 26, 2015
Merged

PHP 5.6: Use variadics and argument unpacking #80

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/Utils/Callback.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ public static function closure($callable, $m = NULL)

self::check($callable);
$_callable_ = $callable;
return function () use ($_callable_) {
return call_user_func_array($_callable_, func_get_args());
return function (...$args) use ($_callable_) {
return $_callable_(...$args);
};
}

Expand All @@ -55,10 +55,10 @@ public static function closure($callable, $m = NULL)
* Invokes callback.
* @return mixed
*/
public static function invoke($callable)
public static function invoke($callable, ...$args)
{
self::check($callable);
return call_user_func_array($callable, array_slice(func_get_args(), 1));
return call_user_func_array($callable, $args);
}


Expand Down Expand Up @@ -87,13 +87,13 @@ public static function invokeSafe($function, array $args, $onError)
if ($file === __FILE__ && $onError(str_replace("$function(): ", '', $message), $severity) !== FALSE) {
return;
} elseif ($prev) {
return call_user_func_array($prev, func_get_args());
return $prev(...func_get_args());
Copy link
Copy Markdown
Member

@dg dg Aug 26, 2015

Choose a reason for hiding this comment

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

Here is $prev(...) fine.

}
return FALSE;
});

try {
$res = call_user_func_array($function, $args);
$res = $function(...$args);
restore_error_handler();
return $res;

Expand Down
2 changes: 1 addition & 1 deletion src/Utils/Image.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,7 +610,7 @@ public function __call($name, $args)
}
array_unshift($args, $this->image);

$res = call_user_func_array($function, $args);
$res = $function(...$args);
return is_resource($res) && get_resource_type($res) === 'gd' ? $this->setImageResource($res) : $res;
}

Expand Down
2 changes: 1 addition & 1 deletion src/Utils/Json.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ public static function decode($json, $options = 0)
if (!defined('JSON_C_VERSION') || PHP_INT_SIZE === 4) { // not implemented in PECL JSON-C 1.3.2 for 64bit systems
$args[] = JSON_BIGINT_AS_STRING;
}
$value = call_user_func_array('json_decode', $args);
$value = json_decode(...$args);

if ($value === NULL && $json !== '' && strcasecmp(trim($json, " \t\n\r"), 'null') !== 0) { // '' is not clearing json_last_error
$error = json_last_error();
Expand Down
6 changes: 3 additions & 3 deletions src/Utils/Strings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,10 +335,10 @@ public static function compare($left, $right, $len = NULL)
* @param string|array
* @return string
*/
public static function findPrefix($strings)
public static function findPrefix(...$strings)
{
if (!is_array($strings)) {
$strings = func_get_args();
if (is_array($strings[0])) {
$strings = $strings[0];
Copy link
Copy Markdown
Contributor

@JanTvrdik JanTvrdik Aug 24, 2015

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.

Copy link
Copy Markdown
Contributor Author

@Majkl578 Majkl578 Aug 24, 2015

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?

Copy link
Copy Markdown
Contributor

@foxycode foxycode Aug 24, 2015

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

Copy link
Copy Markdown
Contributor

@kukulich kukulich Aug 25, 2015

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

Copy link
Copy Markdown
Member

@dg dg Aug 26, 2015

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.

Copy link
Copy Markdown
Contributor

@JanTvrdik JanTvrdik Aug 26, 2015

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?

Copy link
Copy Markdown
Member

@dg dg Aug 26, 2015

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.

Copy link
Copy Markdown
Contributor Author

@Majkl578 Majkl578 Aug 26, 2015

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?

}
$first = array_shift($strings);
for ($i = 0; $i < strlen($first); $i++) {
Expand Down