Add a confirm form for feed delete

Indeed, without the patch it's possible to simply click the wrong button on the edit page and the feed is gone.

After applying the patch, nothing happens at all, when I click on "Delete". I only return to the Feed aggregator controllpanel and the feed is still there.

thank you, sivaji. You'r right, clearing cache helps.

Patch works for me.

#1: 767894_feed_delete_confirm_1.patch queued for re-testing.

Looks mostly good.

+++ modules/aggregator/aggregator.admin.inc	11 Apr 2010 16:44:35 -0000
@@ -157,8 +159,6 @@ function aggregator_form_feed_validate($
   if ($form_state['values']['op'] == t('Delete')) {

There's more delete-related code in the submit handler.

+++ modules/aggregator/aggregator.admin.inc	11 Apr 2010 16:44:35 -0000
@@ -198,6 +186,39 @@ function aggregator_form_feed_submit($fo
+  $path = (arg(0) == 'admin') ? 'admin/config/services/aggregator' : 'aggregator/sources';
...
+  $form_state['redirect'] = (arg(0) == 'admin') ? 'admin/config/services/aggregator/' : 'aggregator/sources/';

We should default to the administrative path here, and pass the 'destination' query parameter in the other (front-end) case.

+++ modules/aggregator/aggregator.admin.inc	11 Apr 2010 16:44:35 -0000
@@ -198,6 +186,39 @@ function aggregator_form_feed_submit($fo
+  return confirm_form($form, t('Are you sure you want to delete the feed %name ?', array('%name' => $feed->title)), $path, t('This action cannot be undone.'), t('Delete'));

Can we write the confirm_form() function arguments on multiple lines, like elsewhere?

+++ modules/aggregator/aggregator.test	11 Apr 2010 16:44:36 -0000
@@ -41,6 +41,7 @@ class AggregatorTestCase extends DrupalW
+    $this->drupalPost(NULL, NULL, t('Delete'));

Second NULL should be array().

Powered by Dreditor.

#7: 767894_feed_delete_confirm_form_7.patch queued for re-testing.

Rerolled.

When trying to apply this patch I get an error

git apply 767894-feed_delete_confirm_form-12.patch
error: patch failed: core/modules/aggregator/aggregator.admin.inc:68
error: core/modules/aggregator/aggregator.admin.inc: patch does not apply
error: patch failed: core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorTestBase.php:61
error: core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorTestBase.php: patch does not apply

I tested this on the build from 3/28/13. The problem still exists

#1946324: Convert confirm_form() in aggregator.admin.inc to the new form interface just completely redid aggregator confirm_forms(). Please use that as a guide, with routes and ConfirmFormBase

I'm happy to push this ahead.

I propose to:

• Use the new ConfirmFormBase
• Add a new operation on the feed 'Delete' (see attached)
• Remove the 'Delete' link from the Feed edit page (see attached)

why remove delete button? we should keep it and just redirect to confirm page deletion route, like in most places in core

@rootatwc Much better idea!

Looking through the EntityFormController to work out where this link gets rendered, and how to change it...

Changes as per #16. except did not remove delete link from edit form as per #17.

Note, this mostly a rewrite from #12 as it uses the new FormInterface.

Kim

good job! just two things and i think this is ready

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedFormController.phpundefined
@@ -139,15 +139,7 @@ public function save(array $form, array &$form_state) {
+    $form_state['redirect'] = array('/admin/config/services/aggregator/delete/feed/' . $feed->id());

this does not have to be array since you are not passing additional stuff like query.
Also preceding slash should be removed

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/FeedDelete.phpundefined
@@ -0,0 +1,74 @@
+use Drupal\Core\Form\ConfirmFormBase;
+
...
+  public function buildForm(array $form, array &$form_state, Feed $aggregator_feed = NULL) {

you forgot use-ing the Feed object, typehint will fail

Fixes as per #20

Updated tests to use new delete path.

Woah. That last patch included stuff it wasn't supposed to. Probably because it wasn't rebased of the latest 8.x branch. Let's try this one. Interdiff is the same.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedFormController.phpundefined
@@ -139,15 +139,7 @@ public function save(array $form, array &$form_state) {
+    $form_state['redirect'] = '/admin/config/services/aggregator/delete/feed/' . $feed->id();

this should be
'admin/config/services/aggregator/delete/feed/' . $feed->id()
:)

Fix for #25

#26: 767894-aggregator-confirm-form-26.patch queued for re-testing.

looks good to go now, failure was random
thanks!

Looks great, thanks!

Committed and pushed to 8.x.