Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ACF’s picture

Status: Active » Needs review
FileSize
12.1 KB
amateescu’s picture

Updated the patch as an example of how things have to look like after #1934832: Provide a dedicated approach for using forms in routes got committed. What the interdiff is missing is that we no longer need to register a service for the new form class.

amateescu’s picture

Issue tags: +SprintWeekend2013

Add sprintweekend tag.

Crell’s picture

Status: Needs review » Needs work
+++ b/core/modules/update/update.module
@@ -170,8 +170,7 @@ function update_menu() {
-    'page callback' => 'drupal_get_form',
-    'page arguments' => array('update_settings'),
+    'page callback' => 'NOT_USED',
     'access arguments' => array('administer site configuration'),

This should now be a route key, specifying the route name.

Other than that, this looks fine.

ACF’s picture

Status: Needs work » Needs review
FileSize
16.91 KB

A re-roll with that change.

Crell’s picture

Status: Needs review » Needs work
+++ b/core/modules/update/lib/Drupal/update/Form/UpdateFormBase.php
@@ -0,0 +1,46 @@
+/**
+ * Provides a generic locale form class.
+ */
+abstract class UpdateFormBase extends SystemConfigFormBase implements ControllerInterface {

Uh, whut? Why are you defining a class that duplicates SystemConfigFormBase and then not using it...?

ACF’s picture

Status: Needs work » Needs review
FileSize
10.67 KB

Oops left in some old code.

amateescu’s picture

Status: Needs review » Needs work
+++ b/core/modules/update/lib/Drupal/update/UpdateSettingsForm.php
@@ -0,0 +1,124 @@
+  public function validateForm(array &$form, array &$form_state) {
+    parent::validateForm($form, $form_state);

Seems a bit weird that in all the other conversions we had the calls to parent methods at the end of the methods and here is at the top, can we move this one as well?

ACF’s picture

Status: Needs work » Needs review
FileSize
10.67 KB

Made change.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Looking good now :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.