create a forum, save.
Try then to edit > delete it.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Confirmed. Though this does seem to work from the admin/category interface.

edmund.kwok’s picture

Title: Cannot delete forum. » Cannot delete forum/container.
Status: Active » Needs review
FileSize
3.36 KB

Another one of those codes that gets left behind with drupal_set_form's new syntax. The edit forum page is a callback to drupal_get_form() with the argument 'forum_form_forum'. The delete operation is checked in forum_form_forum() and returns a confirm form. The issue here is that the confirm form array not rendered, as in not passed to drupal_get_form.

It should be noted that the same problem exists with deleting containers so the title of this issue was changed accordingly.

This patch does these:
1. Changes callback of adding/editing forum and container page to forum_form_main()
2. forum_form_forum() would determine which form to render, including the confirm delete form
3. forum_form_forum() and forum_form_container() are only responsible for returning the form array for adding/editing forums and containers

You may need to reset your menu before the patch can work. Please review.

webchick’s picture

Problem is now fixed with the patch.

However, should we be doing $form_values 'op' and 'confirm' rather than $_POST? Referencing your other patch here: http://drupal.org/node/83397 which looks like it was received favourably.

eaton’s picture

webchick, it appears that this situation is slightly more complicated. The code in question is doing its work in the page-building function, rather than the submit handler itself. Thus, it has no access to $form_values and peeks directly at $_POST. That's more of an underlying workflow problem with this set of forms, and should probalby be tackled separately once the 'blocking defect' is fixed...

webchick’s picture

Status: Needs review » Reviewed & tested by the community

Fair enough. :)

edmund.kwok’s picture

FileSize
4.12 KB

Ah, I was wondering how I would explain the situation of the code. A few other modules also follow a similar workflow, and we can only use $_POST. I agree with Eaton and we should definitely look into how this can be improved.

Anyway, one slight change to the patch; the Cancel link points to admin/content/forums instead of admin/content/forum. Apart from that, this patch should be good to go :-)

Dries’s picture

Shouldn't we use the _validate + _submit hooks here?

drumm’s picture

Status: Reviewed & tested by the community » Needs review

Instead of overloading that callback, the delete page should have a separate menu item.

edmund.kwok’s picture

I think what Eaton said in #7 could applied to both Dries' and drumms's concerns.

Dries:
If I'm reading it right, in addition to what Eaton said, using _submit and _validate hooks in this issue is not possible with the current workflow. The confirm delete function returns a form array and the _submit and _validate hooks don't return rendered forms.

drumm:
I think for the time being, this patch is the best way to solve the issue as reported. Like Eaton said, we should look into the issue of reworking the workflow of deleting forums (e.g. separate menu callback, avoiding the use of $_POST) separately. I think comment and user multi delete also follow the same workflow and could benefit from a separate menu callback, but IMO should be left for another issue.

edmund.kwok’s picture

Improving delete forum/container workflow using a separate menu callback is addressed in http://drupal.org/node/85539

drumm’s picture

Status: Needs review » Fixed

Committed to HEAD.

Anonymous’s picture

Status: Fixed » Closed (fixed)