When i add a new taxonomy term (in page admin/structure/taxonomy/%tid/add) the term gets saved, a status message like "Created new term term-name" is displayed and the same term adding form is rendered without clearing the form data. It may tempt the user to click the save button again result it duplicate terms.
In my opinion it should either redirect the user to term listing page (admin/structure/taxonomy/%tid to show that the term has been added successfully) or clear the input form (in page admin/structure/taxonomy/%tid/add, assuming the user has more terms to add)
D6 assumes the user has more term to add.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 641314_2.patch | 536 bytes | naxoc |
| #13 | 641314_1.patch | 457 bytes | naxoc |
| #7 | 641314.patch | 673 bytes | naxoc |
| #2 | 641314-clear-term-form.patch | 840 bytes | bleen |
Comments
Comment #1
bleen commentedI can confirm this issue and it's not good ... I'll see if I cant patch
Comment #2
bleen commentedI'm not 100% certain this is the correct way to tackle this, but its simple and to the point ... and it seems to work fine.
Comment #3
grndlvl commentedI believe this would break the multistep process maybe if add a check that would see if form is multistep? not sure what correct fix would be for this.
Comment #4
bleen commented@grndlvl ... can you give me a test case? I'm happy to keep going on this, but Im not sure how this might break
Comment #5
sun.core commentedNot sure why this was critical.
Comment #6
mr.baileysMarked #663988: "Add terms" behavior is confusing as a duplicate of this one. I agree with sun that this is not critical
Comment #7
naxoc commentedI couldn't get the patch from #2 to work, but I made a patch that removes some code that does not make sense.
There was a redirect set up to go to
admin/structure/taxonomy- I don't think the form should ever redirect to there. With that removed I could remove a rebuild = TRUE and all tests still pass.Comment #8
naxoc commentedComment #9
sivaji_ganesh_jojodae commented#7 Works great !!
Comment #10
joachim commented$form_state['rebuild'] is undocumented.
I have applied the patch, and I can confirm it fixes the problem.
However, I HAVE NO IDEA WHY.
This kind of fixing stuff by scrabbling around in the dark is not the way to do it. Hence I am not entirely comfortable marking this as RTBC because I don't understand what the patch does -- all TWO LINES of it.
(See #367006: [meta] Field attach API integration for entity forms is ungrokable for more information on $form['rebuild'] -- or rather for no information at all.)
I get the impression that 'rebuild' is needed for some FieldAPI mojo, so we probably can't just rip it out. However it's clearly messing things up...
Comment #11
catch$form_state['rebuild'] = TRUE means that the form will be re-created if there's an ahah submission - i.e. the 'add more' button for unlimited cardinality fields - so this patch would break that.
Probably, taxonomy_form_term_submit() should set $form_state['rebuild'] = FALSE instead - since if we finally submit the term, we have no need to rebuild the form. I don't have time to write that patch now, but if it works, that ought to not break anything even if it's not 100% the proper fix.
Comment #12
joachim commentedIn taxonomy_form_term_submit 'rebuild' is FALSE when we get to the comment ' // Massage #text_format.'
Comment #13
naxoc commentedI set a $form_state['rebuild'] = FALSE in the submit handler now and put the $form_state['rebuild'] = TRUE back in the builder.
Was that what you meant? :)
Comment #14
catchYes - just removing the redirect, and setting rebuild to FALSE in the final submit. Tested this and it works. Probably needs a comment though, and wouldn't mind an extra review from someone more familiar with this bit of fapi/field AP.
Even though other forms always have redirect set, seems like this ought to be applied in general in case that's removed - we never actually want to rebuild the form after this submit do we?
Comment #15
naxoc commentedI put in a comment. I am not sure I understand what you mean by 'applied in general' in the second paragraph?
Comment #16
naxoc commentedOh, and here is the patch
Comment #17
catchLooks good.
By apply in general, I mean we need to apply a consistent pattern to node, comment and user forms too, but that's not for this issue.
Comment #18
joachim commentedI would file a followup issue for that, but I'm not really sure how to explain it :/
Comment #19
catchI think that comes under the entity / form api ungrokkable issue.
Comment #20
andypostMarked as duplicate #722850: On term creation admin/structure/taxonomy/1/add, after submit, fields are still populated with prev term data
Comment #21
bleen commentedComment #22
andypostDuplicate of what issue?
Comment #23
bleen commentedoooopss .. I misread #20. I thought you were trying to mark *this* issue as duplicate of #722850: On term creation admin/structure/taxonomy/1/add, after submit, fields are still populated with prev term data and thought I was bing helpful. My bad
Comment #24
dries commentedWell, I still don't understand why we need
$form_state['rebuild'] = TRUEto begin with. Clearly, the taxonomy module doesn't need it, and if some module wants to have the form rebuild, they could explicitly set rebuild to TRUE, not? Let's discuss this some more ...Comment #25
catch@Dries, it does need it, due to #367006: [meta] Field attach API integration for entity forms is ungrokable - field API cannot function properly without this. This was already pointed out in #10 and #11. Back to RTBC, see you on that issue.
The only difference with taxonomy is that the form doesn't redirect, all the other entity forms do, so you never see the form rebuilt after submit has happened.
Comment #26
catchAlso related #735800: node form triggers form level submit functions on button level submits, without validation.
Comment #27
dries commentedThanks for the clarification. Committed to CVS HEAD.