I would like to see some additional functionality added to the autocomplete widget:
1. Using user-supplied value when user does not use the autocomplete functionality.
Currently the autocomplete only works if the user chooses one of the suggested values so that an entity id is placed behind it in parenthesis.
When a user chooses to ignore the autocomplete and fills in a valid term/name/title/value it is ignored.
2. Validating invalid input on required fields.
When a field is marked as required the autocomplete widget makes invalid entries empty but does this after the form API has checked the required field for being empty. Therefore making an invalid choice result in an empty value in the required field without any errors or warnings.
I have attached my attempt at a patch that addresses these functionalities.
Comment | File | Size | Author |
---|---|---|---|
#43 | 1389238-43.patch | 1.03 KB | sammys |
#42 | 1389238-42.patch | 1.16 KB | jrb |
#32 | 0001-fixed-1389238-entityref-autocomplete.patch | 5.31 KB | geek-merlin |
#31 | 0001-fixed-1389238-entityref-autocomplete.patch | 5.17 KB | geek-merlin |
#30 | 0001-fixed-1389238-30-entityref-autocomplete.patch | 4.93 KB | geek-merlin |
Comments
Comment #1
amitaibu> When a user chooses to ignore the autocomplete and fills in a valid term/name/title/value it is ignored.
The problem is there is no uniqueness. If there are two same node titles, ER can select the correct one, only if there is the entity ID as part of the string. So I think this patch should deal only with (2).
Comment #2
JvE CreditAttribution: JvE commentedHm, I think the non-uniqueness of labels is another problem altogether.
Since the selection presented to the user does not differentiate between entities with the same label either, the chance that a user selects the "correct" one is the same as the chance that ER gets it right.
i.e. if a user types 'app' and is presented with the choice between 'apple', 'apple' and 'apple' you basically have the same situation. (and presenting them with the choice between 'apple (1)', 'apple (5)' and 'apple (12563)' is really not going to help ;)
The patch ensures that when a user types 'apple' they at least get a reference to an apple if one exists (expected behaviour) and not a silently ignored empty field (unexpected behaviour).
I made this patch for me because my customers were complaining that typing in titles they knew did not work (they can type a lot faster than autocomplete can keep up) and the logs were complaining that there were missing items in fields marked as required.
I could make it into three patches, one for (1), one for (2) and one for (1+2) if that would help you.
Comment #3
amitaibuYes, I think it's better to split into two separate issues.
Comment #4
JvE CreditAttribution: JvE commentedI created
#1398364: Autocomplete: Required fields are not alidated properly
and
#1398360: Autocomplete: User supplied values are being ignored
Comment #5
ransom CreditAttribution: ransom commentedIt wont error on multiple items/unlimited items past the first. Does work with single entries though.
Comment #6
JvE CreditAttribution: JvE commentedNew patch:
- always errors on invalid input, not only on required fields
- handles multiple value fields.
- errors on ambiguous input
Comment #7
Juryiel CreditAttribution: Juryiel commentedI like this patch as an option for fields but it doesn't work good in all situations.
For example, one of my entity references is a select list that's in a field collection with multiple values such that every time I edit a page, a new set of fields in teh field collection appears. If I select -None- (default) this patch will complain and won't let me edit any of the other fields on the node that are not part of this field collection. However, if I select a value, it will obviously add a new set of field-collection fields to the node.
I'm nto sure if that was clear, but basically it makes it impossible to edit any part of a node with a multiple value field-collection that contains an entity reference field without adding an additional set of fields from that collection.
Comment #8
JvE CreditAttribution: JvE commentedYou are correct. The validation is way too broad, it should limit itself to autocomplete fields.
I'll see if I can whip up a new patch tomorrow.
Comment #10
JvE CreditAttribution: JvE commentedI'm learning more every day..
Comment #11
JvE CreditAttribution: JvE commentedComment #12
Juryiel CreditAttribution: Juryiel commentedI tried the patch again and it doesn't seem to let me just type a node that I know exists as described. It seems to have standard function where if I type in a node without teh corresponding node id number, it will leave the field blank.
Comment #13
JvE CreditAttribution: JvE commented@juryiel : that is the standard behaviour without my patch. Can you doublecheck that the patch is actually applied?
Comment #14
Juryiel CreditAttribution: Juryiel commentedI'll try and check later today, but when I applied it I clearly remember it said it patched 2 uh.. heaps? chunks? 2 units of something successfully.
Comment #15
Juryiel CreditAttribution: Juryiel commentedSorry about the delay. I checked and the patch was indeed applied. Is there anything additional I have to do other than patch the files? If not, it's still not working, just typing in something doesn't seem to do anything different than the default behavior on my end (the nodes do exist).
Comment #16
Juryiel CreditAttribution: Juryiel commentedI also tested, and sometimes it will give me a server error when entering a node without its id number in parentheses. I'm not sure what the conditions for this are but it seems to be that the first time I attempt to save the form it will just go back to teh form edit screen without a value saved. Any subsequent times it will return a 500 error and will continue to do that until I enter nothing in the field or enter a node with its id in parentheses.
Comment #17
JvE CreditAttribution: JvE commentedDue to changes Damien made to the module the patch from #10 no longer works.
I will have to make a new patch.
Comment #18
JvE CreditAttribution: JvE commentedNew patch with minor changes to follow changes in the module.
Comment #20
JvE CreditAttribution: JvE commented#18: entityreference-autocomplete-1389238-18.patch queued for re-testing.
Comment #22
JvE CreditAttribution: JvE commentedBah, first the test-bot git crashes and now I've got windows line endings. New patch.
Comment #24
JvE CreditAttribution: JvE commentedComment #25
Juryiel CreditAttribution: Juryiel commentedHad a chance to test the patch and it seems to be working. In my brief tests. As I use it more I'll report any issues.
Comment #26
JvE CreditAttribution: JvE commentedSo far no issues reported by my clients either.
Comment #27
JvE CreditAttribution: JvE commentedLatest entityreference code bypasses the patch's check for invalid values in required fields.
Comment #28
JvE CreditAttribution: JvE commentedNew leaner and cleaner patch, works on RC-1.
- support for values not entered through autocomplete (no-js or fast user)
- disallows references to non-existing entities
Fixes bugs #1398364: Autocomplete: Required fields are not alidated properly and #1398360: Autocomplete: User supplied values are being ignored.
Comment #29
geek-merlinGood work JvE!!!
Just found this issue while working on creating an entityrefcreate module (like noderefcreate).
think what this patch needs:
* the patch only works in _entityreference_autocomplete_validate(), it should ALSO work in _entityreference_autocomplete_tags_validate()
also i have the gut feeling that the logic should live in a handler to be pluggable
(but i'm not sure where)
Comment #30
geek-merlinhere is an improved patch that
* also works for autocomplete-tags-style
* makes the logic pluggable
i also have the feeling that the common logic in _entityreference_autocomplete_validate() & _entityreference_autocomplete_tags_validate() should be factored out, but do not want to make changes even more complex.
and the tests should be written to get this in.
Comment #31
geek-merlinsorry, a minor omission, this is the right one.
Comment #32
geek-merlinagain a minor improvement: added needed pass by reference
Comment #33
JvE CreditAttribution: JvE commentedStill works like a charm.
Comment #34
chasingmaxwell CreditAttribution: chasingmaxwell commentedAre there plans to commit this patch? I'm hoping to utilize it for a project, but am hesitant if it wont be making its way into a released version of Entity Reference.
Comment #35
baby.hack CreditAttribution: baby.hack commentedI'm also wondering if this will be committed. I need this functionality, and would prefer an *official* release.
Comment #36
Damien Tournoud CreditAttribution: Damien Tournoud commentedMerged into 7.x-1.x, thanks!
Comment #38
mfoda CreditAttribution: mfoda commentedHas this patch been merged into the rc3 release?
User input validation for required fields was working for me when entity reference was referencing a vocabulary (gave an error: entity X does not exist). When I switched to an entity reference view it stopped validating. I tried applying the patch but it gave me "reversed or previously applied patch detected."
Any ideas how to fix this?
Comment #39
morbiD CreditAttribution: morbiD commentedmfoda's problem in #38 possibly related to the issue I just opened: #1819618: Incorrect autocomplete validation with views selection mode
The validation for the "Views" selection mode needs to be updated, similar to the "Simple" selection mode.
Comment #40
heatherwoz CreditAttribution: heatherwoz commentedI am noticing the behavior described in #1398360: Autocomplete: User supplied values are being ignored, which this issue's patch was supposed to fix. It's kind of strange, because I have multiple sites using required entityreference fields, and only recently have I noticed that users are able to create nodes without a value in those fields. I am able to reproduce it by typing a valid value (node title) without selecting the option that pops up in the autocomplete. When I save, the new node is created but the entityreference field has no value. I am not using the views selection mode.
Comment #41
lionfish-dupe CreditAttribution: lionfish-dupe commentedI customised the module somewhat to hide the (nid) brackets and to allow the functionality described. I found it didn't seem to work if the field was referencing users, rather than nodes and was being filled in using a hook_form_alter with a username. Deleting the added username and typing it by hand makes it work.
Anyway, the code below makes it work either way - thought it might be useful to post here for reference.
It's a bit hacky (e.g. probably need to replace the db_query with field access functions etc?). It works for me. I'm not sure why the current 7.x-1.0 version doesn't work with my site - I'll have to compare the code, not had a chance yet.
Comment #42
jrbThe "Using user-supplied value when user does not use the autocomplete" functionality was broken with this commit:
http://drupalcode.org/project/entityreference.git/blobdiff/428f635411e75...
The format of the array returned by getReferencableEntities() was changed. The attached patch fixes the problem for me-- it just puts the array back in the same format that it used to be in.
I'm not sure if getReferencableEntities() can ever return two different types to entities, but my patch would cause a problem if two different entities had the same id.
Comment #43
sammys CreditAttribution: sammys commentedThe if statement is unnecessary since the called function always returns an array. Here is a re-rolled patch. #1472596: Free tagging entity reference to a vocabulary does not create the term, only allows existing terms depends on this patch.
Comment #44
jetwodru CreditAttribution: jetwodru commentedHi,
Are these included in the latest version ? If not, cumulative patch or just apply the latest patch only ? I'm a bit confused, thanks
Comment #45
jennypanighetti CreditAttribution: jennypanighetti commentedSame question from jet six months ago - is this resolved yet? Is this still an issue?
Comment #46
AdamPS CreditAttribution: AdamPS commentedThis issue is for a completed feature request and its title and category reflect that. I suspect many people have not noticed that a bug is being discussed in the last few comments. I have created a new issue to cover the bug that has been subsequently introduced #2375957: Autocomplete widget allows manual entry, but it fails and can corrupt data
Comment #47
AdamPS CreditAttribution: AdamPS commentedTo tidy up - I'm marking this as fixed because the original issue was committed years ago, and we have the new related issue I opened to track the subsequent bug.