Better field rules collection, with AfterCollectingFieldRules event

[rudiedirkx]

My apps have custom fields and custom validators, but I want to use the form builder as pure as possible. An event for altering field rules would be perfect. Event BeforeFormValidation is too late, because it has prepared the entire validator already.

Patch is almost completely backward compatible, but not quite. The return type of FormField::getValidationRules() has changed from array to Rules. For new fields that's fine (because Rules::fromArray), but if a developer extends an existing field and expects an array from parent::getValidationRules(), that will crash.

[rudiedirkx]

I'm not a fan of fireEvent(), but I don't know how else to call the event dispatcher. FormHelper has a (protected) member $formBuilder, but it's always null, never set, never used.

[kristijanhusak]

@rudiedirkx looks good. Would maybe implementing ArrayAccess for Rules solve the backward compatibity issue?

[rudiedirkx]

Only for very simple access like $x = $rules['rules'];. I wouldn't encourage $rules['rules']['name'][] = 'min:10'; etc. By keeping Rules opaque, it can become more useful in the future. I think this small bc break (chance) is acceptable.

Maybe add some kind of deprecation notice where it encounters an array instead of a Rules? I don't know how that would work...

[rudiedirkx]

Another use for this new event is #402. A developer can add an in:... rule to every ChoiceType automatically. I know I'm gonna.

[kristijanhusak]

Ok, let's leave it like this. Thanks for PR.

[rudiedirkx]

This is pretty bad code... I blame you too Kris!

To add a single max:255 rule to a field if it doesn't have a max rule yet:

$events->listen(AfterCollectingFieldRules::class, function(AfterCollectingFieldRules $event) {
  $field = $event->getField();
  $ruler = $event->getRules(); // instanceof Rules

  $name = $field->getNameKey();
  $rules = $ruler->getRules()[$name] ?? []; // getRules() is keyed by field name

  foreach ($rules as $rule) {
    if (strpos($rule, 'max:') === 0) {
      return;
    }
  }

  // To add it, whaaaat:
  $ruler->append(['rules' => [$name => [count($rules) => 'max:255']]]);
});

I have to explicitly add the $name, AND the rule's position in the field rules!?!? Because append() uses array_replace_recursive() which doesn't smart-merge numeric arrays like array_merge.

Slight more readable:

  $rules[] = 'max:255';
  $ruler->append(['rules' => [$name => $rules]]);

but still way too explicit. So I'm adding a few helper methods to Rules, which is why it exists:

$ruler->getFieldRules($fieldname = null);
$ruler->addFieldRule($rule, $fieldname = null);
$rules->setFieldRules(array $rules, $fieldname = null);

And $fieldName can be optional by remembering the field/origin when creating it (in FormHelper->getFieldValidationRules().

I should beta test more next time, before PR'ing.

[rudiedirkx]

Huh...

remote: error: GH006: Protected branch update failed for refs/heads/master.
remote: error: Required status check "continuous-integration/travis-ci" is expected.

I can't push to master? How do I make changes? I thought the Collaborator point was to not need PRs for everything..? I've pushed to a separate branch: https://github.com/kristijanhusak/laravel-form-builder/pull/new/better-rules-collection-methods

[kristijanhusak]

@rudiedirkx try again, i removed restriction.

[rudiedirkx]

Maybe that was on purpose. I don't know your desired flow.

Push worked!, and I removed the temp branch. I'll still make PRs for significant features, but this seemed small enough.

Agree? Disagree? @mikeerickson too?

[mikeerickson]

@rudiedirkx I was going to approve this PR when I got home, but it has already been done.

[rudiedirkx]

Yes, this PR is approved a while ago. But it was incomplete, so I wanted to complete the feature separately, without PR, quickly.

I don't know what the best flow is for a multi dev project. Every commit can't be a PR. But sometimes a review is a good thing.

[mikeerickson]

@rudiedirkx Well, if we need to have a second set of eyes, just perform the typical PR request a PR review? Am i missing something?

[kristijanhusak]

@rudiedirkx @mikeerickson lets always create PRs and have someone review it.
Push to master only as an emergency and tag releases.

[mikeerickson]

Sounds good to me, I actually like it this way better. I like the workflow that requires peer code review, if for anything to catch even little issues.