-
Notifications
You must be signed in to change notification settings - Fork 28
[5.3] Closure vs callable #25
Comments
I will try to look into this as soon as possible and come up with a PR as a proposal. |
As noted in that ticket in laravel framework, there are BC issues when it comes to overwriting these classes, as method signatures have to match exactly. Taylor mentioned in that ticket that he's fine with doing it "where BC isn't affected" - that probably means only for classes that aren't likely to be overwritten. |
How about doing the switch for 5.4? ping @GrahamCampbell |
Hi, i followed each topic that is associated with the closure vs callable and it's what brought me here. I also vote for the callable, but I might have led by the wrong idea, so we need advice. I made class for custom validator registration <?php
namespace App\Validators;
abstract class CustomValidator
{
/**
* Validator message when error occure
* @var string
*/
protected $message = null;
/**
* Enter point for validator registration
* @return CustomValidator
*/
public static function register()
{
$called_class = get_called_class();
$validator = new $called_class;
$validator->boot();
}
/**
* Register validator and replacer
*/
abstract public function boot();
} And here is example of implementation <?php
namespace App\Validators;
use Illuminate\Support\Facades\Validator;
class CsvAllowCustomValidator extends CustomValidator
{
/**
* Validator message when error occure
* @var string
*/
protected $message = "Field :field_name doesn't contain allowed values.";
public function boot()
{
/* This works fine
Validator::extend('csvAllow', 'App\Validators\CsvAllowCustomValidator@extend', $this->message);
Validator::replacer('csvAllow', 'App\Validators\CsvAllowCustomValidator@replacer');
*/
// But this won't work besause is not Closure
Validator::extend('csvAllow', [$this, 'extend'], $this->message);
Validator::replacer('csvAllow', [$this, 'replacer']);
}
public function extend($attribute, $value, $parameters, $validator)
{
$csvArray = explode(',', $value);
return !count(array_diff($csvArray,$parameters));
}
public function replacer($message, $attribute, $rule, $parameters)
{
return str_replace(':field_name', $attribute, $message);
}
} Is this good approach and reason for using callable? |
I think that this PR it's so good not only for BC compatibility:
thanks |
The upcoming Laravel 5.5 being a huge step (PHP 7 required, LTS version), it would be great to make the switch to callable in this release. |
There's a only a few days left until 5.5 is released. Is there enough time to write the PR and test it for backward compatibility? This is probably a larger change and could target 5.6, at earliest. |
@sisve Closure is callable, I think that there will not issues... but agree with you that it's better to avoid this kind of "inoffensive" changes now. Maybe 5.5.1 ;) |
Arrays and strings can also be callables. This is the fun part and affects all places where a parameter can be either a Closure or an array/string. So, Container::bind has the signature Another place is What about There are many places where callable make sense, but there's also a lot of places that checks Even if we're only talking about type hints in method signatures, and not part of the docblocks, it involves a lot of breaking changes. Everyone that has implemented an interface or overridden a method that is changed in this issue is forced to update their code. A package that has a custom PasswordBroker implementation (as an example) will have problem implementing a method with one signature in the old code, and one signature in the new code. There are lots of backward-compatibility breaks involved when changing method signatures. |
Hey good folks,
I have a question, related to a design problem I stumbled upon today while developing some nice code with Laravel:
Why don't we use
callable
type-hint instead of\Closure
wherever applicable? The former covers the latter, whereas the other way around isn't valid. Example code:won't work with Laravel's current signatures. However if one changed these sigs, let's say for the
group()
method above FROMpublic function group(array $attributes, \Closure $callback)
TO
public function group(array $attributes, callable $callback)
everything would be honkie dorie. We could use both Closures and regular callables. Additionally, let me note that
callable
type-hinting has been introduced in PHP 5.4, so we are covered. As you can see there are no BC implications involved as well.Thoughts?
CC: @taylorotwell , @GrahamCampbell
The text was updated successfully, but these errors were encountered: