Multiple parent relations confirmation form loses term description

Two notice messages fixed.

Just the right file name

So, we have two PHP notices to deal with here:

1. When you add a term with multiple parents for the first time in a vocabulary. That is, on the screen where you are asked if you want to "Set multiple term parents?". This warning is in confirm_form()
2. When you clicked the "Set multiple parents" button on the screen mentioned above we get an notice in taxonomy_form_term_submit_build_taxonomy_term(). This also makes the term description disappear if you previously saved the term without multiple parents.

The attached patches are in the wrong format I think. It also has some code styling issues:

+++ b/system/system.module
@@ -2815,13 +2815,17 @@ function _system_sort_requirements($a, $b) {
   $description = isset($description) ? $description : t('This action cannot be undone.');
-
+  ¶
   // Prepare cancel link.

Whitespace here...

+++ b/system/system.module
@@ -2815,13 +2815,17 @@ function _system_sort_requirements($a, $b) {
     $options = $path;
+    // To be sure that $options['path'] will be set ¶
+    if (isset($options['destination']) && !isset($options['path'])) {
+      $options['path'] = $options['destination'];  ¶
+    }
   }

...and some here.

I'm bumping this to critical since content is lost. I'll take this patch for another round.

Powered by Dreditor.

@dixon_: I'm taking a look at this issue, confirm it reproduces on latest from 7.x branch. Are you here in chicago in a code sprint room? I'm in core room.

I've confirmed the data loss and notice in problem #2 @dixon_ reported above. I didn't see the first warning.

I'm not sure the patch is the right way to fix the problem. The form seems to be rebuilt incorrectly in this case. I'll step through it again to try to figure out why it's happening.

I'm using a fresh pull & install -- commit 34c6cdd66bac4c7b0c262cc2873c88a65f7b591c.

Tagging.

OK, the problem here is really in confirm_form() wiping out $form['description'].

How should we fix this? Maybe confirm_form() should use $form['confirm_form_description'] ? But I'm betting a bajillion contrib modules (even core?) rely on it being $form['description']. So maybe the taxonomy admin code needs to stuff 'name' and 'description' etc not directly into the $form array.

Whoops.

The patch for #5 removed the confrim_form error but there is still the other error taxonomy_form_term_submit_build_taxonomy_term(). Please check the confirm form screenshot that is appearing here. I did not understand why url_alias field added to the confirm form. I am looking into this.

Just to clarify I think @azriprajwala is talking about the confirmation form having notice. The patch at #5 doesn't fix the data loss issue.

Tagging...again.

Just wrote a patch that fixes the problem for me. dixon_ helped me clarify the comment, and he is going to add his test case to test it (and in general test this function of core).

Ok, here is a patch that does three things:

1. Fixes PHP notice in confirm_form() from patch #3 by @a.mikheychik
2. Fixes the data loss bug caused by confirm_form() overwriting the term description from patch in #14 by @adamdicarlo.
3. Adds a test for this multi step form that occurs when triggering the multiple parents option for a vocabulary.

Overall, this feels a little bit hacky. Theoretically confirm_form() could overwrite form values everywhere, especially when they share keys as common as "description".

I don't know if we need to look into a better solution for this? The solution in this patch changes as few things as possible in the confirm_form() definition.

What was already in the module code looks like an ugly hack to me:

  // Check for confirmation forms.
  if (isset($form_state['confirm_delete'])) {
    return array_merge($form, taxonomy_term_confirm_delete($form, $form_state, $term->tid));
  }
  elseif (isset($form_state['confirm_parents'])) {
    return array_merge($form, taxonomy_term_confirm_parents($form, $form_state, $vocabulary));
  }

For starters, why can't taxonomy_term_confirm_FOO do it's own array merging?

Second, in general, whaaaa? It's one form and then suddenly it effectively becomes another? I've just greped for '_confirm_' and I don't see this pattern anywhere else in core.

Third, it's broken anyway. If you enable path module, then the term alias field shows on the confirmation form. That will happen presumably with anything that form_alters term forms.

So yeah, I think we need to look into a better solution for this :/

@joachim, yeah, the form becoming another form is a definite WTF; and I figured we'd fix the term alias field showing with a separate patch. Can we really *rewrite* this for 7.x? My attempt with the patch was to modify as little code as possible....

The problem we have at the moment is that anything doing a hook_form_alter on this form has to account for the two alternative versions of the form in addition to the main one -- term deletion confirmation and multiple parent confirmation.

They they are stuck in a double bind:

- a) don't add your elements to the form in the multiple parent case: your data will not be saved!
- b) don't account for the alternate version of the form: your form elements erroneously show on the multiple parent confirmation page.

The term description element is doing a), and path module's path alias field is doing b).

So I think this does need a rewrite, as it's completely broken.

However, the problem to solve is this: what's the right pattern in FormAPI to have a confirmation page for saving something? AFAIK we only confirm deletions in Drupal core.

Huh. I was expecting FieldAPI fields on terms to lose their data the same way, but they behave correctly.

Though I also get a variety of error messages, such as:

Error message
Notice: Undefined index: path in confirm_form() (line 2848 of /Users/joachim/Sites/7-drupal/modules/system/system.module).

Moving to 8.x and adding the backport tag.

Subscribing

Oh well if this on D8 now, we can do the decent thing and rewrite this total mess of a form :)

For Drupal 7, I think we should just remove the confirm_form() from here, it's not necessary and was added extremely late in the Drupal 6 cycle, that's a UI change, but we'd just be removing a page, not adding anything new, it will stop the data loss, and it's rarely encountered anyway.

Then we can open a new issue to make confirm_form() itself non-destructive, that might be worth backporting to D7 but it feels more major bug than critical to me since without taxonomy we're not sure any other modules are running into it at all.

OK here's patch that does that.

I used dixon's test verbatim - apart from the one line submitting the multiple parents confirmation form which is removed. The rest of the changes just remove the confirmation form.

So the change here is that if you select two parents for a taxonomy term, instead of a form saying "did you really want to do that?", it just lets you do it.

The specific test dixon patches passes locally but I didn't run everything.

I'm sure this is in D6 too so adding tag for that version.

This should deal with the data loss, we can then open another issue to ensure confirm_form() is non-destructive in general.

+++ b/modules/taxonomy/taxonomy.admin.inc
@@ -873,25 +864,6 @@ function taxonomy_form_term_submit_build_taxonomy_term($form, &$form_state) {
-  $description = '<p>' . t("Adding multiple parents to a term will cause the %vocabulary vocabulary to look for multiple parents on every term. Because multiple parents are not supported when using the drag and drop outline interface, drag and drop will be disabled if you enable this option. If you choose to have multiple parents, you will only be able to set parents by using the term edit form.", array('%vocabulary' => $vocabulary->name)) . '</p>';

Could we repurpose some of this text for the vocabulary term list, so multiple hierarchy vocabs explain why the drag & drop is not present? I know that will probably be a string change, but it would reduce UI surprise...

(Though I note in passing that the drag handles are still appearing for me, just the save button is gone.... but that's another issue.)

Powered by Dreditor.

Marked #1015572: Notice: Undefined index description taxonomy_form_term_submit_build_taxonomy_term(), line 842 of taxonomy.admin.inc as duplicate.

...coming from #1015572: Notice: Undefined index description taxonomy_form_term_submit_build_taxonomy_term(), line 842 of taxonomy.admin.inc

I had the same question as joachim in #25, but seems it already does that currently, I assume this help text is not changed by this patch:

I reproduced the bug and the warning screen was rather puzzling, so no objections to removing it from a UX perspective: just let people do it if they want to, and the next screen even provides actionable info on how to undo it and bring back the drag-n-drop (though indeed, the handles are still there)

Reading the patch, in the test it says "Term parents was successfully saved." Should be "Term parents were successfully saved." ?

> seems it already does that currently, I assume this help text is not changed by this patch:

Ah, I should have checked it out first... I withdraw my objection in that case :)

> Reading the patch, in the test it says "Term parents was successfully saved." Should be "Term parents were successfully saved." ?

Yup, I think it should.

Fixed was/where in the test.

I never intended to assign this to myself :) But I will definitely revisit and test this one very soon. Seems like some good progression has been made here, since I was involved.

I've opened #1190284: Make confirm_form() non-destructive for the API change to confirm_form().

#30: taxonomy-multiple-949616.patch queued for re-testing.

Re-rolled.

Subscribing. I can reproduce the same error messages as described by the OP and in #19, using Drupal 7.4 with either PostgreSQL 9.0.4 or SQLite 3.3.7.

Confirmed behaviour from #1 on Drupal 8, patch from #35 resolves the problem and the term appears as child of both selected parents. RTBC!

Issue summary:

Since Drupal 6, taxonomy module has tried to automatically calculate whether you're using a multiple parents vocabulary or not (previously this was a setting on the vocabulary admin form with three different options). Because multiple parents are a bit janky (and because having them disables drag and drop since that can't handle the same term two places in the tree), there was also a confirmation form added for if you were going to switch your vocabulary to that setting. This was all added last minute during D6 rc with the series of drag and drop patches that went in.

Core doesn't have a pattern for confirmation forms except when doing things like deletion, and the existing code here uses a bit of a strange one (turn the existing form into a confirm form).

Because both the term form and the confirm form have a description key, the confirm form was destroying data from the taxonomy form, hence the data loss and notices.

There is already a drupal_set_message() from submitting the form, to tell people why drag and drop was disabled, so the wtf-ish-ness of this is not really made any better or worse by removing the confirm form. IMO we shouldn't have added the confirmation firm at all, and it is probably a relection of how much the original people involved in that drag and drop issue (which includes definitely me and quicksketch) hate multiple parent vocabularies rather than being strictly necessary.

This means the patch can just remove a few lines of code (apart from adding tests).

Previous patches that tried to keep the confirm form, had to work around the double description thing which works but is a bit hacky.

This means the current patch here is a UI change, but it is only removing a confirmation screen that users see very rarely, if ever.

If the UI change is not acceptable for D7, then we could commit #15 as is. The tests are identical and that is likely the best option otherwise.

Awesome, thanks for the summary.

I agree with the approach taken by #35. I don't think this UI change will be noticed by anyone unless it's to say "Oh thank goodness that doesn't happen anymore." It's also very unlikely to invalidate documentation, since it's such a weird use case.

Committed and pushed #35 to 8.x and 7.x. Thanks!