Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mtift’s picture

Issue tags: +WSCCI-conversion

Anything that's "take something from hook_menu and make a route of it" should get the WSCCI-conversion tag

kim.pepper’s picture

Assigned: Unassigned » kim.pepper
Issue tags: -WSCCI-conversion

Assigning to me.

kim.pepper’s picture

Issue tags: +WSCCI-conversion

Damn. Didn't mean to remove tag.

kim.pepper’s picture

Status: Active » Needs review
FileSize
4.55 KB

Initial patch that does this. :-)

tim.plunkett’s picture

The conversion looks good, just some nitpicks.

+++ b/core/modules/action/lib/Drupal/action/Form/DeleteForm.phpundefined
@@ -0,0 +1,77 @@
+<?php
+/**
+ * @file

Missing blank line

+++ b/core/modules/action/lib/Drupal/action/Form/DeleteForm.phpundefined
@@ -0,0 +1,77 @@
+   * @var \stdClass $action

the $action is redundant

+++ b/core/modules/action/lib/Drupal/action/Form/DeleteForm.phpundefined
@@ -0,0 +1,77 @@
+   * Implements \Drupal\Core\Form\ConfirmFormBase::getQuestion().

Here and everywhere else: {@inheritdoc}

+++ b/core/modules/action/lib/Drupal/action/Form/DeleteForm.phpundefined
@@ -0,0 +1,77 @@
+    drupal_set_message(t('Action %action was deleted', array('%action' => $label)));
+
+    $form_state['redirect'] = 'admin/config/system/actions/manage';
+  }

Blank line after the end of the method before the class

kim.pepper’s picture

Rebased off latest 8.x and applied fixes in #5

Status: Needs review » Needs work
Issue tags: -FormInterface, -WSCCI-conversion

The last submitted patch, 1946318-action-confirm-form-6.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
Issue tags: +FormInterface, +WSCCI-conversion

#6: 1946318-action-confirm-form-6.patch queued for re-testing.

andypost’s picture

+++ b/core/modules/action/action.admin.incundefined
@@ -6,38 +6,6 @@
-function action_admin_delete_form($form, &$form_state, $action) {

+++ b/core/modules/action/action.routing.ymlundefined
@@ -18,3 +18,11 @@ action_admin_configure:
+  pattern: 'admin/config/system/actions/delete/{action}'

+++ b/core/modules/action/lib/Drupal/action/Form/DeleteForm.phpundefined
@@ -0,0 +1,78 @@
+  protected $action;
...
+  public function buildForm(array $form, array &$form_state, $action = NULL) {
+
+    $this->action = action_load($action);

$action should always exists. No reason to call action_load() - router item does it

+++ b/core/modules/action/lib/Drupal/action/Form/DeleteForm.phpundefined
@@ -0,0 +1,78 @@
+    $aid = $this->action->aid;
+    $label = $this->action->label;
...
+    action_delete($aid);
...
+    watchdog('user', 'Deleted action %aid (%action)', array('%aid' => $aid, '%action' => $label));
+    drupal_set_message(t('Action %action was deleted', array('%action' => $label)));

please use $this->action - no reason in new variables

kim.pepper’s picture

1) Discussed this with @andypost in IRC and agreed that actions are not entities, so don't get autoloaded.

2) Removed extra variables

andypost’s picture

Status: Needs review » Reviewed & tested by the community

There's no need to introduce new param converter for single form! Should be green

catch’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -WSCCI-conversion

Committed/pushed to 8.x, thanks!

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