Problem/Motivation

Expression forms that have a validateForm() method are calling submitForm(). This is bad design. It's calling that method to do something a method like extractFormValues() is supposed to do.

This happens in these expression forms:

  • \Drupal\rules\Form\Expression\ActionForm
  • \Drupal\rules\Form\Expression\ConditionForm

The implementation of the expression forms should follow a similar pattern as the the form methods of RulesConfigurableEventHandlerInterface.

Secondly, \Drupal\rules\Form\EditExpressionForm::validateForm() does not call validateForm() on the expression form, and it should be.
This way multi-step forms can do custom validation of steps easily.

Proposed resolution

  1. Make sure that ActionForm::validateForm() and ConditionForm::validateForm() are calling extractFormValues() instead of submitForm().
  2. Move the portion of submitForm() that validateForm() needs to extractFormValues().
  3. Make sure that EditExpressionForm::validateForm() calls validateForm() on the expression form.

Remaining tasks

User interface changes

API changes

Data model changes

Original report by fago

submitForm is getting called during validateForm, because it really is extractFormValues(). This should be aligned with form methods of RulesConfigurableEventHandlerInterface(). However, it should continue to have a validateForm() method imho, such that multi-step forms can do custom validation of steps easily.

Comments

fago created an issue. See original summary.

fago’s picture

Category: Task » Bug report
fago’s picture

Priority: Normal » Critical

Bumping prio as it's an API change.

fago’s picture

Issue tags: +beta blocker
fago’s picture

Issue tags: +Contributor
kmox83’s picture

Assigned: Unassigned » kmox83
dasjo’s picture

Assigned: kmox83 » Unassigned

Unassigning as it hasn't been touched in a while, thanks to anyone for picking it up

TR’s picture

If someone is looking for a way to contribute, this is a nice self-contained task that's not only critical but also a beta blocker. It would be really useful to get this fixed!

TR’s picture

Bump. This is a beta blocker because we know it will definitely involve an API change, and the API should remain unchanged between beta and stable release as much as possible.

TR’s picture

Category: Bug report » Task
Priority: Critical » Normal

This is not critical. It works as-is. In fact it's not even a bug, just a poor implementation.

Marking it as a beta blocker is sufficient to emphasize its importance. We need to correct this implementation before beta because it is an API change.

MegaChriz’s picture

Issue summary: View changes

This issue report is also a bit confusing to me. I made a guess of what's wrong and what need to be fixed and wrote that down in the issue summary. Can you check if I parsed the information correctly so far?

TR’s picture

@MegaChriz: I think what you said in the issue summary is correct.

I would also like to add something which wasn't mentioned by @fago but which I noticed a while ago. Namely, ExpressionFormInterface defines validateForm() and submitForm(), so implementations of ExpressionFormInterface *seem* to be just like "real" FormInterface forms, but they're not - ExpressionFormInterface does NOT extend FormInterface, and ExpressionFormInterface::validateForm has a different signature than FormInterface::validateForm (ExpressionInterfaceForm::validateForm uses array $form instead of array &$form like FormInterface)

Because ExpressionFormInterface is so close to, yet not quite the same as, FormInterface, it's easy to assume the wrong thing when reading or writing code that uses ExpressionFormInterface.

Personally, I would do one of two things:
1) Make ExpressionFormInterface extend FormInterface, so it behaves exactly the same way. Then we can make assumptions like the validate function should call the parent::validate, and we can assume the build -> validate -> submit workflow just like the core Form API.
or
2) Make ExpressionFormInterface significantly different from FormInterface, so there is no confusion. For example, rename validateForm() to validate() or validateExpressionForm() or something, likewise with submitForm().

In the issue summary when you say:

Secondly, \Drupal\rules\Form\EditExpressionForm::validateForm() does not call validateForm() on the expression form, and it should be.

This is an example of how confusing ExpressionFormInterface is. EditExpressionForm implements FormInterface, and NOT ExpressionFormInterface. Inside EditExpressionForm::buildComponent we get a reference to an expression and call ExpressionFormInterface::submitForm here. That sounds wrong, but ExpressionFormInterface is not really a Form API form so we're really not submitting the form here either ...

Also, in EditExpressionForm::validateForm() we don't actually do anything with the ExpressionFormInterface at all, but we do call ::buildComponent() which calls ExpressionFormInterface::submitForm() but not ExpressionFormInterface::validateForm(). So yeah, the logic here is very confusing.

This is one of the problems I was having when working on #2916331: Passing variables from rules to components and back. I'll try to post my work-in-progress to that issue so you can see how I was trying to work around this.