Skip to content

Commit e115feb

Browse files
nilmergnilmerg
committed
Cleanup form error handling
Forms rely mostly on event handling right now but that has a limitation regarding errors in the framework at the moment. The previous work-around now lead to the problem that exceptions upon submit were not rendered as form errors but silently ignored and the form just didn't submit at all without any explanation. The new way to solve the originating problem is to always render form errors but still override `onError` in forms as there is a case in which it is called but no error event is emitted: In case element validation fails. It is now possible to only rely on `Form::ON_ERROR` to ensure the client's DOM is updated to reflect the state of the form properly.
1 parent 7d54b1c commit e115feb

14 files changed

Lines changed: 52 additions & 53 deletions

‎application/controllers/ChannelController.php‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,11 +58,9 @@ public function indexAction(): void
5858
}
5959

6060
$this->redirectNow('__CLOSE__');
61-
})->on(Form::ON_SENT, function (ChannelForm $form) {
61+
})->on(Form::ON_ERROR, function ($_, ChannelForm $form) {
6262
// TODO: I feel this should be part of CompatForm or CompatController (e.g. $this->sendForm())
63-
if (! $this->getResponse()->isRedirect()) {
64-
$this->addPart($form, $this->content->getAttribute('id')->getValue());
65-
}
63+
$this->addPart($form, $this->content->getAttribute('id')->getValue());
6664
})->handleRequest($this->getServerRequest());
6765
}
6866
}

‎application/controllers/ChannelsController.php‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -122,11 +122,9 @@ public function addAction(): void
122122
)
123123
);
124124
$this->switchToSingleColumnLayout();
125-
})->on(Form::ON_SENT, function (ChannelForm $form) {
125+
})->on(Form::ON_ERROR, function ($_, ChannelForm $form) {
126126
// TODO: I feel this should be part of CompatForm or CompatController (e.g. $this->sendForm())
127-
if (! $this->getResponse()->isRedirect()) {
128-
$this->addPart($form, $this->content->getAttribute('id')->getValue());
129-
}
127+
$this->addPart($form, $this->content->getAttribute('id')->getValue());
130128
})->handleRequest($this->getServerRequest());
131129
}
132130

‎application/controllers/ContactController.php‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -62,11 +62,9 @@ public function indexAction(): void
6262
));
6363

6464
$this->redirectNow('__CLOSE__');
65-
})->on(Form::ON_SENT, function (ContactForm $form) {
65+
})->on(Form::ON_ERROR, function ($_, ContactForm $form) {
6666
// TODO: I feel this should be part of CompatForm or CompatController (e.g. $this->sendForm())
67-
if (! $this->getResponse()->isRedirect()) {
68-
$this->addPart($form, $this->content->getAttribute('id')->getValue());
69-
}
67+
$this->addPart($form, $this->content->getAttribute('id')->getValue());
7068
})->handleRequest($this->getServerRequest());
7169
}
7270

‎application/controllers/ContactGroupController.php‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -92,14 +92,8 @@ public function editAction(): void
9292
$this->switchToSingleColumnLayout();
9393
} elseif (! $form->hasBeenSubmitted() && ! $form->hasBeenDuplicated()) {
9494
foreach ($form->getPartUpdates() as $update) {
95-
if (! is_array($update)) {
96-
$update = [$update];
97-
}
98-
9995
$this->addPart(...$update);
10096
}
101-
} else {
102-
$this->addPart($form, $this->content->getAttribute('id')->getValue());
10397
}
10498
})
10599
->on(Form::ON_SUBMIT, function (ContactGroupForm $form) {
@@ -120,6 +114,10 @@ public function editAction(): void
120114
$this->closeModalAndRefreshRemainingViews(Links::contactGroup($group->id));
121115
}
122116
})
117+
->on(Form::ON_ERROR, function ($_, ContactGroupForm $form) {
118+
// TODO: I feel this should be part of CompatForm or CompatController (e.g. $this->sendForm())
119+
$this->addPart($form, $this->content->getAttribute('id')->getValue());
120+
})
123121
->handleRequest($this->getServerRequest());
124122
}
125123
}

‎application/controllers/ContactGroupsController.php‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -157,14 +157,8 @@ public function addAction(): void
157157
->on(Form::ON_SENT, function (ContactGroupForm $form) {
158158
if (! $form->hasBeenSubmitted() && ! $form->hasBeenDuplicated()) {
159159
foreach ($form->getPartUpdates() as $update) {
160-
if (! is_array($update)) {
161-
$update = [$update];
162-
}
163-
164160
$this->addPart(...$update);
165161
}
166-
} else {
167-
$this->addPart($form, $this->content->getAttribute('id')->getValue());
168162
}
169163
})
170164
->on(Form::ON_SUBMIT, function (ContactGroupForm $form) {
@@ -177,6 +171,10 @@ public function addAction(): void
177171
$this->getResponse()->setHeader('X-Icinga-Container', 'col2');
178172
$this->redirectNow(Links::contactGroup($groupId));
179173
})
174+
->on(Form::ON_ERROR, function ($_, ContactGroupForm $form) {
175+
// TODO: I feel this should be part of CompatForm or CompatController (e.g. $this->sendForm())
176+
$this->addPart($form, $this->content->getAttribute('id')->getValue());
177+
})
180178
->handleRequest($this->getServerRequest());
181179
}
182180

‎application/controllers/ContactsController.php‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -139,11 +139,9 @@ public function addAction(): void
139139
Database::get()->transaction(fn(Connection $db) => (new ContactRepository($db))->create($contact));
140140
Notification::success($this->translate('New contact has successfully been added'));
141141
$this->switchToSingleColumnLayout();
142-
})->on(Form::ON_SENT, function (ContactForm $form) {
142+
})->on(Form::ON_ERROR, function ($_, ContactForm $form) {
143143
// TODO: I feel this should be part of CompatForm or CompatController (e.g. $this->sendForm())
144-
if (! $this->getResponse()->isRedirect()) {
145-
$this->addPart($form, $this->content->getAttribute('id')->getValue());
146-
}
144+
$this->addPart($form, $this->content->getAttribute('id')->getValue());
147145
})->handleRequest($this->getServerRequest());
148146
}
149147

‎application/controllers/ScheduleController.php‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -189,10 +189,6 @@ public function addRotationAction(): void
189189
$form->on(Form::ON_SENT, function ($form) {
190190
if (! $form->hasBeenSubmitted()) {
191191
foreach ($form->getPartUpdates() as $update) {
192-
if (! is_array($update)) {
193-
$update = [$update];
194-
}
195-
196192
$this->addPart(...$update);
197193
}
198194
}
@@ -269,10 +265,6 @@ public function editRotationAction(): void
269265
$this->closeModalAndRefreshRelatedView(Links::schedule($rotation->scheduleId));
270266
} elseif (! $form->hasBeenSubmitted() && ! $form->hasBeenDuplicated()) {
271267
foreach ($form->getPartUpdates() as $update) {
272-
if (! is_array($update)) {
273-
$update = [$update];
274-
}
275-
276268
$this->addPart(...$update);
277269
}
278270
}

‎application/controllers/SourceController.php‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -57,11 +57,9 @@ public function indexAction(): void
5757
));
5858

5959
$this->switchToSingleColumnLayout();
60-
})->on(Form::ON_SENT, function (SourceForm $form) {
60+
})->on(Form::ON_ERROR, function ($_, SourceForm $form) {
6161
// TODO: I feel this should be part of CompatForm or CompatController (e.g. $this->sendForm())
62-
if (! $this->getResponse()->isRedirect()) {
63-
$this->addPart($form, $this->content->getAttribute('id')->getValue());
64-
}
62+
$this->addPart($form, $this->content->getAttribute('id')->getValue());
6563
})->handleRequest($this->getServerRequest());
6664
}
6765

@@ -97,11 +95,9 @@ public function deleteAction(): void
9795
Notification::success($this->translate('Deleted source successfully'));
9896
$this->switchToSingleColumnLayout();
9997
})
100-
->on(Form::ON_SENT, function (DeleteSourceForm $form) {
98+
->on(Form::ON_ERROR, function ($_, DeleteSourceForm $form) {
10199
// TODO: I feel this should be part of CompatForm or CompatController (e.g. $this->sendForm())
102-
if (! $this->getResponse()->isRedirect()) {
103-
$this->addPart($form, $this->content->getAttribute('id')->getValue());
104-
}
100+
$this->addPart($form, $this->content->getAttribute('id')->getValue());
105101
})
106102
->handleRequest($this->getServerRequest());
107103
}

‎application/controllers/SourcesController.php‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -113,11 +113,9 @@ public function addAction(): void
113113
Notification::success(sprintf(t('Added new source %s successfully'), $source->name));
114114
$this->switchToSingleColumnLayout();
115115
})
116-
->on(Form::ON_SENT, function (SourceForm $form) {
116+
->on(Form::ON_ERROR, function ($_, SourceForm $form) {
117117
// TODO: I feel this should be part of CompatForm or CompatController (e.g. $this->sendForm())
118-
if (! $this->getResponse()->isRedirect()) {
119-
$this->addPart($form, $this->content->getAttribute('id')->getValue());
120-
}
118+
$this->addPart($form, $this->content->getAttribute('id')->getValue());
121119
})
122120
->handleRequest($this->getServerRequest());
123121
}

‎application/forms/ChannelForm.php‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -437,7 +437,12 @@ public function validate(): static
437437

438438
protected function onError()
439439
{
440+
parent::onError();
441+
440442
// TODO: I feel like this should be the case in ipl-html already
441-
$this->emit(Form::ON_SENT, [$this]);
443+
if (! $this->hasMessages()) {
444+
// Trigger the event in case only validation failed
445+
$this->emit(Form::ON_ERROR, [null, $this]);
446+
}
442447
}
443448
}

0 commit comments

Comments
 (0)