From d7104d0ae1266972aaa647b483e00e806e1e7619 Mon Sep 17 00:00:00 2001 From: Timm Ortloff Date: Wed, 1 Feb 2023 12:27:08 +0100 Subject: [PATCH 1/5] Remove multiple submit button hack and replace `setId()` with `fromId()` as a static factory method --- application/controllers/ReportController.php | 20 ++++----- application/controllers/ReportsController.php | 9 ++-- library/Reporting/Web/Forms/ReportForm.php | 41 ++++++++++--------- 3 files changed, 36 insertions(+), 34 deletions(-) diff --git a/application/controllers/ReportController.php b/application/controllers/ReportController.php index 8139284e..8d2fd6ab 100644 --- a/application/controllers/ReportController.php +++ b/application/controllers/ReportController.php @@ -67,12 +67,12 @@ public function editAction() $values[$name] = $value; } - $form = new ReportForm(); - $form->setId($this->report->getId()); - $form->populate($values); - $form->handleRequest(ServerRequest::fromGlobals()); - - $this->redirectForm($form, 'reporting/reports'); + $form = ReportForm::fromId($this->report->getId()) + ->on(ReportForm::ON_SUCCESS, function () { + $this->redirectNow('reporting/reports'); + }) + ->populate($values) + ->handleRequest(ServerRequest::fromGlobals()); $this->addContent($form); } @@ -84,13 +84,13 @@ public function sendAction() Environment::raiseExecutionTime(); Environment::raiseMemoryLimit(); - $form = new SendForm(); - $form + $form = (new SendForm()) ->setReport($this->report) + ->on(SendForm::ON_SUCCESS, function () { + $this->redirectNow("reporting/report?id={$this->report->getId()}"); + }) ->handleRequest(ServerRequest::fromGlobals()); - $this->redirectForm($form, "reporting/report?id={$this->report->getId()}"); - $this->addContent($form); } diff --git a/application/controllers/ReportsController.php b/application/controllers/ReportsController.php index c52a26c9..f1018f3f 100644 --- a/application/controllers/ReportsController.php +++ b/application/controllers/ReportsController.php @@ -95,10 +95,11 @@ public function newAction() $this->assertPermission('reporting/reports'); $this->addTitleTab($this->translate('New Report')); - $form = new ReportForm(); - $form->handleRequest(ServerRequest::fromGlobals()); - - $this->redirectForm($form, 'reporting/reports'); + $form = (new ReportForm()) + ->on(ReportForm::ON_SUCCESS, function () { + $this->redirectNow('reporting/reports'); + }) + ->handleRequest(ServerRequest::fromGlobals()); $this->addContent($form); } diff --git a/library/Reporting/Web/Forms/ReportForm.php b/library/Reporting/Web/Forms/ReportForm.php index 919302a7..25e2accc 100644 --- a/library/Reporting/Web/Forms/ReportForm.php +++ b/library/Reporting/Web/Forms/ReportForm.php @@ -17,16 +17,26 @@ class ReportForm extends CompatForm use Database; use ProvidedReports; - /** @var bool Hack to disable the {@link onSuccess()} code upon deletion of the report */ - protected $callOnSuccess; - protected $id; - public function setId($id) + /** + * Create a new form instance with the given report id + * + * @param $id + * + * @return static + */ + public static function fromId($id): self { - $this->id = $id; + $form = new static(); + $form->id = $id; + + return $form; + } - return $this; + public function hasBeenSubmitted(): bool + { + return $this->hasBeenSent() && ($this->getPopulatedValue('submit') || $this->getPopulatedValue('remove')); } protected function assemble() @@ -86,28 +96,19 @@ protected function assemble() ]); $this->registerElement($removeButton); $this->getElement('submit')->getWrapper()->prepend($removeButton); - - if ($removeButton->hasBeenPressed()) { - $this->getDb()->delete('report', ['id = ?' => $this->id]); - - // Stupid cheat because ipl/html is not capable of multiple submit buttons - $this->getSubmitButton()->setValue($this->getSubmitButton()->getButtonLabel()); - $this->callOnSuccess = false; - $this->valid = true; - - return; - } } } public function onSuccess() { - if ($this->callOnSuccess === false) { + $db = $this->getDb(); + + if ($this->getPopulatedValue('remove')) { + $db->delete('report', ['id = ?' => $this->id]); + return; } - $db = $this->getDb(); - $values = $this->getValues(); $now = time() * 1000; From 801b40878eb3f5d3a9c2104e68bd1020364beb0a Mon Sep 17 00:00:00 2001 From: Timm Ortloff Date: Wed, 1 Feb 2023 12:30:56 +0100 Subject: [PATCH 2/5] Remove multiple submit button hack and replace `setReport()` with `fromReport()` as a static factory method --- application/controllers/ReportController.php | 9 ++-- library/Reporting/Web/Forms/ScheduleForm.php | 51 +++++++++++++------- 2 files changed, 38 insertions(+), 22 deletions(-) diff --git a/application/controllers/ReportController.php b/application/controllers/ReportController.php index 8d2fd6ab..8c33bc75 100644 --- a/application/controllers/ReportController.php +++ b/application/controllers/ReportController.php @@ -99,13 +99,12 @@ public function scheduleAction() $this->assertPermission('reporting/schedules'); $this->addTitleTab('Schedule'); - $form = new ScheduleForm(); - $form - ->setReport($this->report) + $form = ScheduleForm::fromReport($this->report) + ->on(ScheduleForm::ON_SUCCESS, function () { + $this->redirectNow("reporting/report?id={$this->report->getId()}"); + }) ->handleRequest(ServerRequest::fromGlobals()); - $this->redirectForm($form, "reporting/report?id={$this->report->getId()}"); - $this->addContent($form); } diff --git a/library/Reporting/Web/Forms/ScheduleForm.php b/library/Reporting/Web/Forms/ScheduleForm.php index dc659433..d616a229 100644 --- a/library/Reporting/Web/Forms/ScheduleForm.php +++ b/library/Reporting/Web/Forms/ScheduleForm.php @@ -8,6 +8,7 @@ use Icinga\Application\Version; use Icinga\Authentication\Auth; use Icinga\Module\Reporting\Database; +use Icinga\Module\Reporting\Hook\ActionHook; use Icinga\Module\Reporting\ProvidedActions; use Icinga\Module\Reporting\Report; use Icinga\Module\Reporting\Web\Flatpickr; @@ -25,16 +26,26 @@ class ScheduleForm extends CompatForm /** @var Report */ protected $report; + /** @var int */ protected $id; - public function setReport(Report $report) + /** + * Create a new form instance with the given report + * + * @param Report $report + * + * @return static + */ + public static function fromReport(Report $report): self { - $this->report = $report; + $form = new static(); + + $form->report = $report; $schedule = $report->getSchedule(); if ($schedule !== null) { - $this->setId($schedule->getId()); + $form->setId($schedule->getId()); $values = [ 'start' => $schedule->getStart()->format('Y-m-d\\TH:i:s'), @@ -42,19 +53,29 @@ public function setReport(Report $report) 'action' => $schedule->getAction() ] + $schedule->getConfig(); - $this->populate($values); + $form->populate($values); } - return $this; + return $form; } - public function setId($id) + /** + * @param int $id + * + * @return $this + */ + public function setId(int $id): ScheduleForm { $this->id = $id; return $this; } + public function hasBeenSubmitted(): bool + { + return $this->hasBeenSent() && ($this->getPopulatedValue('submit') || $this->getPopulatedValue('remove')); + } + protected function assemble() { $this->setDefaultElementDecorator(new CompatDecorator()); @@ -100,7 +121,7 @@ protected function assemble() $config = new Form(); // $config->populate($this->getValues()); - /** @var \Icinga\Module\Reporting\Hook\ActionHook $action */ + /** @var ActionHook $action */ $action = new $values['action'](); $action->initConfigForm($config, $this->report); @@ -123,16 +144,6 @@ protected function assemble() ]); $this->registerElement($removeButton); $this->getElement('submit')->getWrapper()->prepend($removeButton); - - if ($removeButton->hasBeenPressed()) { - $this->getDb()->delete('schedule', ['id = ?' => $this->id]); - - // Stupid cheat because ipl/html is not capable of multiple submit buttons - $this->getSubmitButton()->setValue($this->getSubmitButton()->getButtonLabel()); - $this->valid = true; - - return; - } } } @@ -140,6 +151,12 @@ public function onSuccess() { $db = $this->getDb(); + if ($this->getPopulatedValue('remove')) { + $db->delete('schedule', ['id = ?' => $this->id]); + + return; + } + $values = $this->getValues(); $now = time() * 1000; From e6d46958df3a67d94dd4af51eb97f3dffaf56cf8 Mon Sep 17 00:00:00 2001 From: Timm Ortloff Date: Wed, 1 Feb 2023 12:32:27 +0100 Subject: [PATCH 3/5] Remove multiple submit button hack and replace `setTemplate()` with `fromTemplate()` as a static factory method --- .../controllers/TemplateController.php | 11 +++-- .../controllers/TemplatesController.php | 10 ++--- library/Reporting/Web/Forms/TemplateForm.php | 40 ++++++++++--------- 3 files changed, 31 insertions(+), 30 deletions(-) diff --git a/application/controllers/TemplateController.php b/application/controllers/TemplateController.php index 349fdaac..d7ced13a 100644 --- a/application/controllers/TemplateController.php +++ b/application/controllers/TemplateController.php @@ -57,12 +57,11 @@ public function editAction() $template->settings = json_decode($template->settings, true); - $form = (new TemplateForm()) - ->setTemplate($template); - - $form->handleRequest(ServerRequest::fromGlobals()); - - $this->redirectForm($form, 'reporting/templates'); + $form = TemplateForm::fromTemplate($template) + ->on(TemplateForm::ON_SUCCESS, function () { + $this->redirectNow('reporting/templates'); + }) + ->handleRequest(ServerRequest::fromGlobals()); $this->addContent($form); } diff --git a/application/controllers/TemplatesController.php b/application/controllers/TemplatesController.php index d1c0d2ab..fe447a26 100644 --- a/application/controllers/TemplatesController.php +++ b/application/controllers/TemplatesController.php @@ -96,11 +96,11 @@ public function newAction() $this->assertPermission('reporting/templates'); $this->addTitleTab('New Template'); - $form = new TemplateForm(); - - $form->handleRequest(ServerRequest::fromGlobals()); - - $this->redirectForm($form, 'reporting/templates'); + $form = (new TemplateForm()) + ->on(TemplateForm::ON_SUCCESS, function () { + $this->redirectNow('reporting/templates'); + }) + ->handleRequest(ServerRequest::fromGlobals()); $this->addContent($form); } diff --git a/library/Reporting/Web/Forms/TemplateForm.php b/library/Reporting/Web/Forms/TemplateForm.php index d18d2f6a..08c377d1 100644 --- a/library/Reporting/Web/Forms/TemplateForm.php +++ b/library/Reporting/Web/Forms/TemplateForm.php @@ -16,9 +16,6 @@ class TemplateForm extends CompatForm { use Database; - /** @var bool Hack to disable the {@link onSuccess()} code upon deletion of the template */ - protected $callOnSuccess; - protected $template; public function getTemplate() @@ -26,18 +23,32 @@ public function getTemplate() return $this->template; } - public function setTemplate($template) + /** + * Create a new form instance with the given report + * + * @param $template + * + * @return static + */ + public static function fromTemplate($template): self { - $this->template = $template; + $form = new static(); + + $form->template = $template; if ($template->settings) { - $this->populate(array_filter($template->settings, function ($value) { + $form->populate(array_filter($template->settings, function ($value) { // Don't populate files return ! is_array($value); })); } - return $this; + return $form; + } + + public function hasBeenSubmitted(): bool + { + return $this->hasBeenSent() && ($this->getPopulatedValue('submit') || $this->getPopulatedValue('remove')); } protected function assemble() @@ -131,23 +142,14 @@ protected function assemble() ]); $this->registerElement($removeButton); $this->getElement('submit')->getWrapper()->prepend($removeButton); - - if ($removeButton->hasBeenPressed()) { - $this->getDb()->delete('template', ['id = ?' => $this->template->id]); - - // Stupid cheat because ipl/html is not capable of multiple submit buttons - $this->getSubmitButton()->setValue($this->getSubmitButton()->getButtonLabel()); - $this->callOnSuccess = false; - $this->valid = true; - - return; - } } } public function onSuccess() { - if ($this->callOnSuccess === false) { + if ($this->getPopulatedValue('remove')) { + $this->getDb()->delete('template', ['id = ?' => $this->template->id]); + return; } From ba36072c18538b4109d3710cc0361357d4915e54 Mon Sep 17 00:00:00 2001 From: Timm Ortloff Date: Wed, 1 Feb 2023 12:33:06 +0100 Subject: [PATCH 4/5] Remove multiple submit button hack and replace `setId()` with `fromId()` as a static factory method --- .../controllers/TimeframeController.php | 14 +++---- .../controllers/TimeframesController.php | 9 +++-- library/Reporting/Web/Forms/TimeframeForm.php | 37 ++++++++++++------- 3 files changed, 35 insertions(+), 25 deletions(-) diff --git a/application/controllers/TimeframeController.php b/application/controllers/TimeframeController.php index 7411cb15..6783cff5 100644 --- a/application/controllers/TimeframeController.php +++ b/application/controllers/TimeframeController.php @@ -34,14 +34,12 @@ public function editAction() ]; - $form = (new TimeframeForm()) - ->setId($this->timeframe->getId()); - - $form->populate($values); - - $form->handleRequest(ServerRequest::fromGlobals()); - - $this->redirectForm($form, 'reporting/timeframes'); + $form = TimeframeForm::fromId($this->timeframe->getId()) + ->on(TimeframeForm::ON_SUCCESS, function () { + $this->redirectNow('reporting/timeframes'); + }) + ->populate($values) + ->handleRequest(ServerRequest::fromGlobals()); $this->addContent($form); } diff --git a/application/controllers/TimeframesController.php b/application/controllers/TimeframesController.php index c434e3ae..99233e03 100644 --- a/application/controllers/TimeframesController.php +++ b/application/controllers/TimeframesController.php @@ -94,10 +94,11 @@ public function newAction() $this->assertPermission('reporting/timeframes'); $this->addTitleTab($this->translate('New Timeframe')); - $form = new TimeframeForm(); - $form->handleRequest(ServerRequest::fromGlobals()); - - $this->redirectForm($form, 'reporting/timeframes'); + $form = (new TimeframeForm()) + ->on(TimeframeForm::ON_SUCCESS, function () { + $this->redirectNow('reporting/timeframes'); + }) + ->handleRequest(ServerRequest::fromGlobals()); $this->addContent($form); } diff --git a/library/Reporting/Web/Forms/TimeframeForm.php b/library/Reporting/Web/Forms/TimeframeForm.php index e83454f6..54f1ccad 100644 --- a/library/Reporting/Web/Forms/TimeframeForm.php +++ b/library/Reporting/Web/Forms/TimeframeForm.php @@ -15,13 +15,28 @@ class TimeframeForm extends CompatForm use Database; use DecoratedElement; + /** @var int */ protected $id; - public function setId($id) + /** + * Create a new form instance with the given report + * + * @param int $id + * + * @return static + */ + public static function fromId(int $id): self { - $this->id = $id; + $form = new static(); - return $this; + $form->id = $id; + + return $form; + } + + public function hasBeenSubmitted(): bool + { + return $this->hasBeenSent() && ($this->getPopulatedValue('submit') || $this->getPopulatedValue('remove')); } protected function assemble() @@ -64,16 +79,6 @@ protected function assemble() ]); $this->registerElement($removeButton); $this->getElement('submit')->getWrapper()->prepend($removeButton); - - if ($removeButton->hasBeenPressed()) { - $this->getDb()->delete('timeframe', ['id = ?' => $this->id]); - - // Stupid cheat because ipl/html is not capable of multiple submit buttons - $this->getSubmitButton()->setValue($this->getSubmitButton()->getButtonLabel()); - $this->valid = true; - - return; - } } } @@ -81,6 +86,12 @@ public function onSuccess() { $db = $this->getDb(); + if ($this->getPopulatedValue('remove')) { + $db->delete('timeframe', ['id = ?' => $this->id]); + + return; + } + $values = $this->getValues(); $now = time() * 1000; From e6c921aef82d0b270cc58031ed3b3b52e02ffdef Mon Sep 17 00:00:00 2001 From: Timm Ortloff Date: Wed, 1 Feb 2023 12:33:32 +0100 Subject: [PATCH 5/5] Controller: Remove obsolete redirect --- library/Reporting/Web/Controller.php | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/library/Reporting/Web/Controller.php b/library/Reporting/Web/Controller.php index 9cb0d734..1123332d 100644 --- a/library/Reporting/Web/Controller.php +++ b/library/Reporting/Web/Controller.php @@ -4,19 +4,8 @@ namespace Icinga\Module\Reporting\Web; -use ipl\Html\Form; use ipl\Web\Compat\CompatController; class Controller extends CompatController { - protected function redirectForm(Form $form, $url) - { - if ( - $form->hasBeenSubmitted() - && ((isset($form->valid) && $form->valid === true) - || $form->isValid()) - ) { - $this->redirectNow($url); - } - } }