Closed (fixed)
Project:
Hierarchical Select
Version:
7.x-3.0-alpha9
Component:
Code
Priority:
Critical
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
28 Sep 2011 at 07:19 UTC
Updated:
19 Jan 2015 at 15:41 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
marcoscano+1. Same happening for me, whith the slight difference that if there are no mandatory fields it saves the node but keeps the form open. When I do complete all the fields and save, the node duplicates.
I'm using 7.x-3.x-dev.
Thanks
Comment #2
Joel MMCC commentedSame here. If the node is new (Add Content), a new copy of the node, complete with a new NID, is created for any click to create a new Taxonomy term at any level, including if I then [Cancel] out of that without actually creating a new Taxonomy term!
If Editing a node, it saves the node (pops up the green Content Saved banner right above the Hierarchical Select instead of its usual place), but does not create a duplicate (presumably unless Create New Revision is on), again on any attempt to add a new term.
This seriously messes up workflow. I don’t want to have to tell my clients any jump-through-hoops workarounds (e.g. save the new node without any Taxonomy, then go back and edit it to set Taxonomy terms to prevent duplicate nodes from being created en masse), especially since the whole point of this module is to make things easier.
Upgrading to “Critical” because this bug effectively makes Hierarchical Select worse than totally useless, being an actual danger to the integrity of site node data (e.g. a customer wants to try out a change to a node, intending to Preview it before Saving it, and also changes Taxonomy — oops! Create New Revision wasn’t on, so now the “trial” change is made permanent and not easily revertible!).
Comment #3
marcoscanoFound these other issues that may be related to this one (or may even be the same issue):
http://drupal.org/node/1312762
http://drupal.org/node/1312286
http://drupal.org/node/1267768
http://drupal.org/node/1270306
Comment #4
marcoscano@Athos and @Joel MMCC
are you using Administration Language module?
I discover this module was being a source of multiple problems, and after disabling it, this issue (and others) completely disappeared.
I hope this info can be of some help
Comment #5
Joel MMCC commented@clanet, I don’t see any module by that name in my system. It was created on and exported from Drupal Gardens.
Here’s a “prettified” version of the output of DRUSH pm-list, showing every module we have:
All are the absolute latest recommended (for most) or .dev (for Calendar, CTools, Date, and Views) versions except that Views has not yet been updated to the 31-Oct-2011 .dev yet, since the MD5 checksum on it mismatched (see my Issue there).
Edited only to “prettify” the Drush pm-list output above.
Comment #6
Athos commentedHi,
@clanet: No i don't use this specific module.
What i have noticed is that this issue occurs ONLY when i am logged in as an authenticated member.
When i am logged in as an administrator the problem described above DOES NOT happen.
It would be extremely complicating (at least to me) to find out if this issue is caused by another installed module. I 've compared the modules @Joel MMCC has installed with the ones i have and 70% are the same so...where do i begin?
regards
Athos
Comment #7
henrijs.seso commentedI have a theory on why this happens
, just theory yet. So ajax is returning [...] correct form_id.Update: Theory is that
hierarchical_select_ajax()is inspired byfile_ajax_upload()which says "As long as the form processing is properly encapsulated in the widget element...". And in case offile_ajax_upload(), it is well encapsuled in widget element and operates on sub-form for that widget. On the other handhierarchical_select_ajax()is operating on full node form and when saving "Create new item form", submit button processes and submits whole node, not just widget. We need to investigate little more of how file.module does it :)Comment #8
henrijs.seso commentedComment #9
danielphenry commentedI'm also seeing this issue when creating new taxonomy terms on a new node. I do not have a file upload in the form in this case. Because of this it is creating duplicates of each node. Makes the module almost unusable.
Comment #10
snyderp commentedHere is a temporary fix. Implement hook_node_validate, which fires after all other form validation happens (including after the taxonomy fields are saved / processed)
In the hook, check to see if we're in an AJAX callaback initiated from the hierarchical_select module, and if so, silently set the node submission to fail (ie mark the form as invalid).
Haven't tested extensively, but is working for me and seems like it should generally work.
Here is a copy-paste snippet, if anyone is interested:
Hope that helps!
Comment #11
Athos commentedIf i understand correctly we just need to paste the above somewhere? if yes then where?
Thanks
Athos
Comment #12
danielphenry commentedsnyderp's workaround http://drupal.org/node/1293166#comment-5237076 (#10 above) seems to work for me.
The error here is that ajax_get_form on line 294 hierarchical_select.module is returning the whole node form. I think this means the ajax is modifying the existing node form instead of embedding a separate form to add a new term. I'm not really experienced with ajax/javascript so I'm not sure exactly how to fix this one. But the real solution would be to embed a separate form for creating a new term instead of modifying the existing form.
This workaround will do for now but in the mean time this needs to be addressed as the critical priority it is despite the workaround.
Athos,
Instead of using hook_node_validate (which requires you to create the node type in the same module) I just used a form alter and added a new validate handler.
You can past this into any custom module. Make sure to replace all occurances of "my_module" with the module name and "{node_type}" with the machine readable name of the node type you have the hierarchy form attached to.
Comment #13
snyderp commentedJust a quick clarification to danielson317's comment above, the hook_node_validate method does not require that the node content type be defined in the module. I'm sure :-)
But, both of these solutions have the exact same effect and use the same strategy.
Comment #14
Anonymous (not verified) commentedYeah, this prevents me from saving nodes completely, because of
...am using the admin language module.
-EDIT- that is the culprit: just dont use the admin language module
Comment #15
lindsayo commentedI'm also having this problem on 6.x-3.7.
It seems like #14 is suggesting that there's another module interfering? I don't have the admin language module on my site. Not sure what else to suspect, but I'll comb through and start disabling things to check.
EDIT - Looks like this was the Ajax module. Disabled, drush cc all, tested the node/add with multiple terms from HS, and works fine (got one node, not several).
Comment #16
MrPhilbert commentedCan you please be more specific. What is added? What is replaced? What line/s?
Or, could you upload the new file with corrections?
Comment #17
Joel MMCC commented@morningtime #14:
No, that isn’t the culprit. I’ve never used that module (see my module list above), and I have the same issue.
Comment #18
Athos commented:)...i am lost!!!
What do i have to do to fix this issue? i tried adding and deleting stuff from hiearchical.module (i am surprised it is still functioning) with no luck. I am not a programmer so if someone can explain in steps what we have to in order to correct this until next update would be great.
Comment #19
MrPhilbert commentedI just figured it out.
The error is in "live preview". There is a javascript file called common_config.js. Part of it is designed to show your selections.
If you look at line #113 (.filter(':not(.create-new-item-input):not(.create-new-item-create):not(.create-new-item-cancel)')), you'll see that it will delay the input until node save.
If you write any javascript, you'll know that the quotes will prevent this from parsing.
Just delete the single quotes and it will work or paste this in its place:
.filter(:not(.create-new-item-input):not(.create-new-item-create):not(.create-new-item-cancel))
Note: If your field is required, you'll still see required field warnings but this won't effect anything. I am working on an if statement to add to fix this.
Please let me know if this works.
Philbert
Comment #20
Athos commented@MrPhilbert,
Hi,
Unfortunately your fix doesnt work on my site.
Please confirm that the file you are talking about is the common_config_form.js and NOT the common_config.js and it is located in sites/all/modules/hierarchical_select/includes
Thanks
Athos
Comment #21
MrPhilbert commentedAfraid you're right. I added existing terms. It also seems that this module has now been abandoned.
Wim Leers is now working for Facebook and has not posted anything on Drupal.org since September (as far as I can tell).
Unless someone (with a lot more expertise than I) can take over this module, it will not look good for Drupal and the open source community.
I really like Drupal but when things like this and the token problems et. al. happen, it makes me worry about the future of Drupal.
Comment #22
Athos commentedYes this is unfortunate. (unfortunate for the project and not for Wim Leers whom I wish the very best). Perhaps we can somehow donate a little bit as an incentive to get this project moving forward?!
I believe that HS is one of the most important modules when creating forms.
Comment #23
MrPhilbert commentedI agree and also wish him the best.
I believe that Facebook pays very well and also keeps their people very busy.
At one point, he was working on a re-write of this as a 4.x release. Maybe someone would like to contact him and see if he is making any progress?
Comment #24
Drupa1ish commented#10 works fine... Relax!
Comment #25
ahwebd commentedhere's a custom module that solves this issue (using #10) and other HS issues (see http://drupal.org/node/1389740):
http://drupal.org/sandbox/ahwebd/1389734
Comment #26
adam_b commented@ahwebd: any chance you could extract your sandbox module and post it here for testing? not all of us have access to git
Comment #27
guypaddock commentedI spent some time debugging this issue and comparing how HS works compared with the File module.
The actual cause is not what @mansspams or @danielson317 theorized about there needing to be a sub-form. I checked and it doesn't look like file fields use any kind of sub-form to do processing; they're just vanilla form widgets / form elements.
Instead, it's actually the fact that FAPI can't determine what button was actually clicked when processing the form, so it assumes it was the "Save" button. You can see this behavior in
includes/form.incon line 1856.The real hint here is that the other callbacks HS uses, such as the one that triggers the "add new item" form to be rendered in the first place or the callback once a user has chosen an item, do not cause the form to validate. Only the "Create" and "Cancel" buttons cause a problem. The process callback only places those buttons in the form array when the HS widget is in "creating new term" mode, and the process callback is fired BEFORE the FAPI evaluates what button was clicked, so they're not available for form processing.
The File module gets around this by always putting the upload controls / buttons in the form, even when the form is not in a state where uploads are accepted, and then enabling or disabling access to the controls in a "pre_render" callback. That didn't work here, so I used an "after_build" callback which seems to have done the trick.
I also noticed that the D7 AJAX API seems to handle some of these issues. Perhaps if HS was ported to use the built-in AJAX callbacks from D7, this problem might not need special handling. I'm not sure; I'm not up-to-speed yet on it. For now, I've just added a "TODO" comment inside
_hs_process_render_create_new_item()to this effect. Hopefully, someone with more D7 AJAX callback experience can look at this.The patch for this is attached. Please review.
Comment #28
guypaddock commentedSetting this back to NW since it looks like this patch breaks the dropbox. With it applied, when you go back and edit a node that has existing terms selected in the dropbox, if you cancel the term add, the dropbox is cleared.
Comment #29
sixelats commentedPatch #27 works well when e.g. Title field is required but gives me the "field is required" error when the HS fields are required.
Comment #30
d.novikov commentedhttp://drupal.org/node/1293166#comment-5691474
the same for me
Comment #31
kartagis#10 works for me smoothly.
Comment #32
wim leersThis is not specific to Taxonomy.
@GuyPaddock: any progress regarding #28?
Comment #33
johankasperi commented#10 did it for me. Thanks!
Comment #34
betawavetom commentedI've found another solution that is working for me.
Here's how I did it:
1) created my own custom module
2) first identified my form ID (using the helpful info on drupal.org/node/651106)
3) implemented the code found on www.elvisblogs.org/drupal/solved-how-bypass-nodevalidate-hookformalter
Code looks like this:
Now when I add a term and the form submits the whole node, there is a 5 minute time delay for when the user can save the form. It's an alternative way to deal with it (and certainly not perfect ~ but it works.)
I don't know if this would create issues elsewhere though.
What do you think?
PS: Hey Wim Leers, I think you've created about the only multiselect solution that feels and looks good. I wish I won the lottery to share with you. :)
Comment #35
David_Rothstein commentedI ran into a bug with the above patch when used inside the Panels IPE (and specifically when used with the Ctools modal dialog). The issue is that the "Cancel" button added by Ctools inside the form wizard wasn't working correctly. After some debugging I found this was due to the form API getting confused between the form's main "Cancel" button and the one added here, and not knowing which was clicked.
Since "Cancel" is a pretty common button name, I think if this patch is going to add it to a whole bunch of forms like that it would make sense to give it a different machine name other than the standard 'op' which Drupal normally uses.
I don't have time right now to roll this into the above patch directly, but the code for it looks something like below (applied on top of the patch in #27):
Comment #36
jramby commentedHi,
I have problem applying the patch #27... I've tried it on both stable and dev version of hierarchical_select.
Comment #37
jramby commentedHi,
I have problem applying the patch #27... I've tried it on both 7.x-3.0-alpha5 and 7.x-3.x-dev version of hierarchical_select.
Thx
Comment #38
molnitza commentedThe patch works without problems with 7.x-3.0-alpha5+9-dev.
Comment #39
oostieRerolled patch @ #27 against DEV and added Davids recommendations @ #35.
Comment #40
jibize commentedThe patch from #39 works only when the "Create" button is clicked for me, I still get the "... field is required." error when the Cancel button is clicked.
Comment #41
cyberranger commentedThe patch in #39 fixes the duplicate saving of the node when a new term is saved. However, if cancel is selected instead, the whole node is saved resulting in a duplicate when the node is saved.
Comment #42
cyberranger commented#12 solved the issue for me.
Comment #43
wolvern commentedNone of the above patches worked for me, but the solution from http://drupal.org/node/1389740 did the job perfectly
Comment #44
dgtlmoon commentedMy two cents: HS is still submitting the node in ' drupal_process_form($form['#form_id'], $form, $form_state);' when you are selecting Create/Update/Cancel operations when managing a new term, so there for the form status has been changed..
which causes the message 'The content on this page has either been modified by another user, or you have already submitted modifications using this form. As a result, your changes cannot be saved.'
Comment #45
dgtlmoon commentedComment #46
dgtlmoon commentedComment #47
dgtlmoon commentedanother option is to use 'shs' module with https://drupal.org/node/1893188
Comment #48
Strycker commented#43 - this thing fix problem perfectly. Thanks for that!
Comment #49
vegardjo commentedJust used a few hours here, what I found out: both solution #10 and #43 works fine for me for 3.0-alpha5, but none on alpha6. I downgraded and all is fine.
Comment #50
vegardjo commented..sorry, correct is: workes on dev and alpha5, not alpha6 for me. Solution from #43 works better that #10
Comment #51
dags commented#39 works for me. I think this is a good approach because it makes better use of the Form API (ie. it uses #limit_validation_errors).
Comment #52
jantoine commentedAfter applying the patch in #39, I was getting a "Received an invalid response from the server." error when clicking on the 'Cancel' button. Reverting the addition inspired by #35 resolved the error. This, however, resulted in the dropbox clearing behavior as described in #28. Changing the 'Cancel' button #type from 'button' to 'submit' resolved the dropbox being cleared issue. This change has also resolved:
In addition to changing the 'Cancel' button #type from 'button' to 'submit', the attached patch also addresses some coding standards issues and has been re-rolled against dev.
Comment #53
azinck commentedI haven't had time to read through all the comments or related issues here. It seems clear to me that the ajax implementation in Hierarchical Select leaves a lot to be desired. It really needs to be refactored to pass in a specific _triggering_element value.
In our case, we're seeing a problem because we're using Admin Language and the default front-end language is Spanish while the default back-end language is English. So the node edit form is built in English when on node/*/edit, but the ajax requests hit the hierarchical_select_ajax path which is not considered an admin path. So on submission form builder rebuilds the form in Spanish. Thus the submitted form has different values as compared to the rebuilt form for any elements that have had their values translated. This causes the code in _form_button_was_clicked() to not be able to determine which form element made the submission.
So we're seeing a bug merely because the word "Update" was translated on our site.
For our use-case the Hierarchical Select widget is never going to be used on any non-admin pages so our band-aid fix is to use hook_admin_paths_alter() to mark hierarchical_select_ajax as an admin path to ensure it always gets built in the same language as the submitted form was built in.
Comment #55
eric.chenchao commentedThe patch in #52 works! So far so good. Thanks @jantoine
Comment #56
andrewmacpherson commentedCan the maintainer clarify the current status of this?
The commit in #54 was released in 7.x-3.0-alpha8, but the issue still says 'needs review'
Comment #57
stefan.r commentedThis patch is not in alpha8 yet, it will be in alpha9 though. For now, I committed this even if it's not the nicest possible solution.
Comment #58
stefan.r commentedComment #59
pianomansam commentedApparently this was fixed in 7.x-3.0-alpha9 but not updated to reflect that.
Comment #60
pianomansam commentedComment #61
heddnOpened #2410351: Admin language & possible data loss as a follow-up to #53.