Fix #8452 Jquery Extraction (attempt 2) #14865
Conversation
klimov-paul
added
severity:BC breaking
|
Looks neat. Is this planned as a first step to keep the whole system intact, but should |
|
@schmunk42 yes, that's the plan. |
| /** | ||
| * @inheritdoc | ||
| */ | ||
| public $clientValidatorMap = [ |
There was a problem hiding this comment.
I suggest to move this array to a separate method getClientValidatorMap() to make it easier to add custom validators to this map, or override existing.
There was a problem hiding this comment.
Or, maybe, create a ClientValidatorFactory and inject it in ActiveFormClientScript.
It will be responsible for creating ClientValidator based on Validator, than it should be easier to add new mappings
There was a problem hiding this comment.
defaultClientValidatorMap() added
| * ```php | ||
| * <?php $form = \yii\widgets\ActiveForm::begin([ | ||
| * 'id' => 'example-form', | ||
| * 'as clientScript' => \yii\jquery\ActiveFormClientScript::class, |
There was a problem hiding this comment.
What are pros compared to extending ActiveFormClientScript from ActiveForm instead of making it a behavior?
There was a problem hiding this comment.
OK. That was 1st approach. Cons are described here: #13839 That's why behaviors.
| @@ -8,7 +8,7 @@ | |||
| namespace yii\captcha; | |||
There was a problem hiding this comment.
Captcha should be moved to separate repository. Script should be adjusted not to use jQuery.
Could be done in separate pull request after this one is merged.
| /** | ||
| * @inheritdoc | ||
| */ | ||
| public function build($validator, $model, $attribute, $view) |
There was a problem hiding this comment.
It could not be built with any other validator than BooleanValidator from core framework so it makes sense to type-hint here and in similar cases.
|
The pull request is big so I did a drawing while reviewing:
Overall, approach makes perfect sense. Sorry that it took that long to review it properly. |
|
@klimov-paul, it is possible to resolve conflicts? Other than that, the approach is good as well as code itself. The only minor thing to consider is #14865 (comment). Ready to merge when conflicts will be resolved. If you're busy, I can allocate time for it and do it myself, just tell me. |
|
Conflicts resolved. |
|
Merged. That was big one. Thank you for taking care of it. |
Migrated from #13839