Sorry if is just my ignorance but I have been checking it twice and I can't find a different explanation that we are in front of a bug or a design decision that IMHO is not the best choice in most of the the cases.
Scenario:
Clean drupal 5.2 (also with 5.1) with no extra modules.
Problem:
When a field is "required", first term is selected by default.
So users can submit the form without taking care of taxonomy that won't be validated.
Required vocabulary isn't required at all... as far it will be filled with first term.
Source:
if (!$blank && !$value) {
// required but without a predefined value, so set first as predefined
$value = $tree[0]->tid;
}
Obvious possible solution:
if (!$blank && !$value) {
// required but without a predefined value, so forcing to check.
$value = '';
}
If user likes default values, they can perform this operation with hook_alters or taxonomy_default module.
Comment | File | Size | Author |
---|---|---|---|
#43 | taxonomy-single-choice-default.patch | 546 bytes | longwave |
#32 | taxonomy-selector.patch | 488 bytes | JirkaRybka |
#25 | mandatory-5x-dev.patch | 1.38 KB | JirkaRybka |
#18 | mandatory-select-2.patch | 1.37 KB | JirkaRybka |
#13 | mandatory_select_0.patch | 1.61 KB | anantagati |
Comments
Comment #1
JirkaRybka CreditAttribution: JirkaRybka commentedYes, I agree that this behavior is wrong.
If anyone is going to provide a patch, it might be worth looking at http://drupal.org/node/122285 - it's a twin of this issue already fixed in Category module, which is sort-of clone from Taxonomy. So the patch for Taxonomy might be quite similar.
Comment #2
drummNeeds a patch file attached.
Comment #3
JirkaRybka CreditAttribution: JirkaRybka commentedOK, I looked into this again. The described behavior (i.e. 'Required' vocabulary being not required at all) still occurs in 6.x-dev. If the user didn't even touch the Taxonomy selector, the first term from the list is grabbed instead of triggering a validation error (which would be the expected behavior, I dare say), so the field is just defaulting somewhere, instead of being really Required.
The same was already fixed in Category module (my patch at http://drupal.org/node/122285 ), and the same fix also works for Taxonomy. I ported my patch, tested it, and seems to be OK.
With this patch, the "blank" line in selector is not suppressed anymore (turned into a message prompting to make a selection now), so the selector defaults to this line rather than any existing term. If user did no selection, we can catch this in validation (depending on config, the value may be zero string, empty array, or array including the zero element), and set a proper form-error on the required element. This also means that the private function _taxonomy_term_select() gets no more called without a $blank value, so there's a bit of cleanup done too.
Needs review ;)
Comment #4
webchickOhhhh enormous, big, blinking, flaming +1 for this functionality.
Now, to the nit-picking. ;)
1. There are some minor code-style errors; ex: "$key=>$terms" needs spaces, etc. Coder module is great for tracking down these kinds of things.
2.
a) We shouldn't yell at people.
b) None should be capitalized.
I'm not sure what we should use in place of -- Please choose! -- ... I did some hunting around on various sites for best practices, and results are mixed. Sometimes it's just "Choose..." sometimes it's "- Select one -" sometimes it's just "-" This is going to quickly devolve into a bike-shed argument. :\
The informal consensus in Drupal is for "- Please choose... -" so let's go with "- Please choose... -" and "- None selected -" as the options.
Comment #5
JirkaRybka CreditAttribution: JirkaRybka commentedThanks for the review and comments!
Attaching a new patch, with the changes included:
---
$key=>$terms
- Thanks for discovering that, I must have been blind to leave that in (came from my old Category patch ported).--- Coder module: Thanks for tip, nice helper :) It found no more in this patch (there are some minor issues in Taxonomy module already, but completely unrelated, so I'm not mixing issues together).
--- The blank options: Ah, yes... I was really unsure about this, so didn't change the existing one before. Now revised per your suggestion. But I removed the three dots, because it didn't look good to me (being inside hyphens already - which in turn are necessary to emphasize difference from regular terms, and so should stay).
Comment #6
mbria CreditAttribution: mbria commentedThanks for the patch. I wanted to upload it, but my development platform is/was full of rubbish :-(
Back to the issue, I also reflect about a "Please choose" option... but it collides with views' filters where this option is added without no value, so I was concerned it could cause collateral damages somewhere else.
Are you sure it's a good idea?
Comment #7
JirkaRybka CreditAttribution: JirkaRybka commentedmbria: I'm not sure I understand Your point, can you elaborate? If some other module is re-using the taxonomy_form() and doesn't want the blank line, it should be easy to just unset ['#options'][0] - which needs to be done for non-required Vocabularies already, as far as I can see, there's really not much difference between "- Please choose -" and already existing "<none>".
The same for _taxonomy_term_select() - which should not be re-used, being a Taxonomy's "private" function, but otherwise it's entirely possible to leave the cleanup out and have this one unchanged.
Generally, we need that (or some other worded) line added, because otherwise browsers return the first term and there's no way to validate anymore.
Comment #8
mbria CreditAttribution: mbria commentedSorry for the delay in my answer. With your comments and rethinking it a little I must say is better you forget my comment. :-)
You are completely right.
Thanks a lot for patching it.
Comment #9
Dries CreditAttribution: Dries commented+ form_set_error("taxonomy][$key", t('You must choose a term from %vocabulary.', array('%vocabulary' => $vocabulary->name)));
The patch looks good. However, the new error message exposes techno-speak to the user. The average visitor might not know what a 'term' is, or is it a generic enough word? Is there anything we can do to make the error message easier for Joe Average?
Other than that, this looks good.
Comment #10
JirkaRybka CreditAttribution: JirkaRybka commentedRe: Dries - I totally agree that 'taxonomy', 'term' and 'vocabulary' are really confusing words for 'Joe Average' (took me half a year to understand myself, being poor on English too), but I thought that these were 'official' terminology for the Taxonomy module, seeing that 'vocabulary' is used in the pre-existing error message too.
Now, given your comment, attaching a new version - or at least an attempt (really, English is not my strength):
You must make a choice for %vocabulary.
The vocabulary name already appears as a heading above the selector, so I guess we can refer to it this way, avoiding the confusing terminology, without introducing another one (confusing too?).
Comment #11
anantagati CreditAttribution: anantagati commentedI am getting PHP notices with taxonomy-required-3.patch when vocabularies are not required on node preview and default values are not selected:
Comment #12
anantagati CreditAttribution: anantagati commentedSame problem during node preview for required vocabularies.
Comment #13
anantagati CreditAttribution: anantagati commentedI made by mistake duplicate issue (http://drupal.org/node/186919) with patch which is working for me.
Attached patch is making this:
If vocabulary is required default value will be '- Please choose -' which will not be pass validation.
If vocabulary is not-required and not multiple choices default value will be '- None selected -', which will pass validation.
When vocabulary has multiple select none of these texts are visible, because it seems that they are not needed.
Comment #14
catchdevadarshan: are you sure those are caused by this patch? There's an outstanding patch for undefined index, and I think the others are there in HEAD as well.
Comment #15
anantagati CreditAttribution: anantagati commentedOh sorry, I see, problem is in HEAD version.
Comment #16
JirkaRybka CreditAttribution: JirkaRybka commentedRe: #11 - I tested with a fresh 6.x-dev (no patch), and these warnings are shown too. (Previewing a node, where the '<none>' is present - i.e. non-required vocabulary - and nothing else selected.)
Any existing issue for that? I guess it happens because the blank line uses value 0, which passes as numeric into further processing, but is not a valid term.
Anyway, it's not introduced by my patch.
Now going to test the other one...
Comment #17
catchproperty of a non-object: http://drupal.org/node/186837
undefined index: http://drupal.org/node/186750
Comment #18
JirkaRybka CreditAttribution: JirkaRybka commentedOK, I tested the alternative patch in #13:
- Applied only with difficulty (a bit weird format) but allright, going on...
- Works as expected :)
My comments:
- The trick with empty string as array-key is a cool idea! It's nicely caught by the 'required' field validation and reported with the standard error message, thus requiring no extra code in validation.
- Also good catch with the multiple select not needing the blank line. Makes things more consistent, users now may not select the blank line accidentally, along with some other lines.
- But however, I quite dislike the fiddle with numeric $blank flags, and texts defined elsewhere than the actual logic. Also wrapping the text inside both '- -' and '< >' seems strange to my eyes.
I examined what will happen, if we just use the empty-string key for *all* blank lines. And the result is: Nothing happens, all works fine. I did a search: The selector is used for the node submitting (as we're discussing here), and for term edits on admin pages (terms parenting and relations). All these cases already accept the empty-key-fashioned form element (I tested). It's not much of a change really, both zero and empty strings are 'empty' values and invalid term id's, and empty field is already returned from multi-selects.
So we can further simplify the patch, passing blank-line labels as before, and just using that empty key.
Attaching the new patch. I tested all I could, all fine (node submitting with vocabulary required/not-required, multiple/single selection, selecting a term, the 'none' line, or nothing; adding a term with single/multiple parenting, selected nothing/'none'/a term, the same for relations). The cool thing is, that the patch is much smaller now, and does the job even better.
But since this is a new approach, we need a new review here.
Comment #19
JirkaRybka CreditAttribution: JirkaRybka commentedThe patch still applies cleanly. And quite literally - needs review! ;)
Comment #20
catchApplies with 3 line offset. I set a vocabulary to required, added a couple of terms, and tried it with both single and multiple select. Much better, simple fix, RTBC.
Comment #21
Gábor HojtsyThank, committed.
Comment #22
webchickAny way this would be considered for 5.x?
Comment #23
webpotato CreditAttribution: webpotato commentedI'm hoping the errant selected category/taxonomy issue is fixed since I have a site where I'd like the multiple select box NOT to automaticlly select a value, but I don't know how to use the mandatory-select-2.patch file (RCS file?) to patch the taxonomy module. Using "patch" doesn't work I guess because the filename is not "taxonomy-required-2.patch". I'm new to this. Any help would be appreciated. Is there another way when using RCS files?
Comment #24
JirkaRybka CreditAttribution: JirkaRybka commentedThe patches here at drupal.org are generally used with the 'patch' utility, to change the relevant Drupal release for testing purposes. I'm on Linux, having the patch utility installed, so to apply the patch above I would download and unpack the corresponding drupal-xxxxx.tar.gz, go to terminal, switch to the just unpacked directory and use 'patch -p0 < filename.patch'.
BUT, this patch here was created for Drupal 6, so it's not likely to apply cleanly to other versions. If you're trying 6.x-dev, then you are now getting drupal-6.x-dev.tar.gz with this patch already applied, which is indicated by the message in #21: "Committed".
More about applying patches: http://drupal.org/patch/apply
Comment #25
JirkaRybka CreditAttribution: JirkaRybka commentedBelieve it or not, #18 applies to 5.x-dev with just a minor fuzz, and works fine (I tested). So re-rolling for 5.x-dev - just removing the fuzz.
But however, this needs to be considered, as it introduces two new UI strings, which I'm not sure is a good idea in a stable branch.
Comment #26
JirkaRybka CreditAttribution: JirkaRybka commentedComment #27
drummCommitted to 5.x.
Comment #28
(not verified) CreditAttribution: commentedAutomatically closed -- issue fixed for two weeks with no activity.
Comment #29
bfsworks CreditAttribution: bfsworks commentedBIG Problem after 5.5 upgrade.
Using Taxonomy Terms as a Views Filter. Even if the exposed filter is set to Optional, the defualt behavior has changed to - Please Choose - as appose to - ALL -
Again the exposed filter is set to "Optional" and taxonomy menu box should not defualt selection to - Please Choose -
The logic should be if a views exposed filter is "Optional" the defualt selection should be ALL. Which in result would include all content if nothing was directly selected.
I could not find any other posts in regards to this one so I would like to address the issue.
The current result is any Optional Views Taxonomy Filters are actually now being required to take action and if a user is not used to taking action on a optional filter then they assume inoperable or that no content matches their criteria.
Comment #30
JirkaRybka CreditAttribution: JirkaRybka commentedSeems like Views play with the selector in some strange way; should just remove the original blank element and insert it's own one. It would be nice to check how is it in Views code (which version you have?), and possibly fix there...?
Otherwise, this sort of thing is why I was unsure on 5.x backport in #25 - changes to stable branch are risky business.
Comment #31
bfsworks CreditAttribution: bfsworks commentedI have used both latest stable and dev of the views module, both with the same effect. I have not found a work around yet. If any one has please post here.
Thanks.
Comment #32
JirkaRybka CreditAttribution: JirkaRybka commentedI took a quick look - the attached patch should help, but it's absolutely untested, try on your own risk. It's rolled against Views 5.x-1.6 Basically it's update for the core change done here (I'm still uncomfortable about this backported to 5.x, but it already happened...)
If it helps, we probably need to open a new issue for Views project, and move the patch there. (Please don't re-categorize this issue to Views, it needs to stay where it is, for future reference. We'll need to close it again, as it was before.)
Comment #33
ju.ri CreditAttribution: ju.ri commentedi have the same problem on my site. the patch did not change the behaviour in any way.
the filters in a views filter block show "all", but the (optional) filter behaves as "please select" and returns no results. please help! can i just undo the applied diff (drupal 5.5, above) as a quick fix?
Comment #34
JirkaRybka CreditAttribution: JirkaRybka commentedDid you try to empty the Views cache table after applying the #32 patch? (I think there's a button for this, somewhere on the administrative UI).
For further discussion about Views, please open a new issue for the Views project (or link to an existing one, if exists).
Comment #35
catchLooks like a related fix (RTBC) here for views: http://drupal.org/node/199675
Comment #36
ju.ri CreditAttribution: ju.ri commentedi did indeed forget to flush the views cache. thanks! i will open an issue in views project now.
Comment #37
drummReverting status since this views issue is being handled elsewhere.
Comment #38
Summit CreditAttribution: Summit commentedHi,
I have somewhat the same thing on hand I think.
I got the following message:
But I want to use the taxonomy term as filter, and non of the vocabularies is required...
Please assist.
Thanks in advance,
Greetings,
Martijn
Comment #39
francoud CreditAttribution: francoud commentedI think that the "Please choose" label should not be displayed if there is only one term to select and it's "required".
Upgrading to drupal 5.4 (as well as 5.5 and 5.6) broke some of my websites - I was using "taxonomy access control" module to give some user access to only one category term, and now they have a "Please choose" label with a single possible choice. This is leading people to a further, unnecessary "click". With drupal 5.3, the single choice was automatically defaulted. I've the same problem without taxonomy access control module really.
That's why now I cant upgrade to 5.4-5.5-5.6... :(
Comment #40
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.
Comment #41
Summit CreditAttribution: Summit commentedHi,
This fixed it for me: http://drupal.org/user/58871
But I think the fix is not committed yet, right?
greetings,
Martijn
Comment #42
webchickIf you mean the Views issue, that's being handled over here: http://drupal.org/node/199675
If you mean something core-related, you'll have to be more specific. This patch is only about taxonomy.module itself, not Views integration with it.
Closing again.
Comment #43
longwaveI have just run into the same problem as #39. My site has some roles that are forced to choose a particular category term through use of Taxonomy Access Control. When there is only the "Please choose" dummy option and one real option that the user can select from, it makes more sense to select the real option by default.
My patch reverses part of mandatory-5x-dev.patch from #25, but only if there are two options in the dropdown and one is blank. The old code appeared to have a minor bug where it defaulted to the first item in the vocabulary, even if that item had been excluded from the option set. My patch selects the correct value from the $options array instead.
It may be preferable to remove the blank option entirely, leaving the single selectable option in the dropdown. To do this, change
$value = array_shift(array_keys($options[0]->option));
tounset($options['']);
Comment #44
chx CreditAttribution: chx commentedComment #45
floretan CreditAttribution: floretan commentedWe probably want this behavior only when the taxonomy is required and the user really wouldn't have an option.
When it is not required, the user has the option between "none" and "First and unique term", and the default should be "none".
The problem is that _taxonomy_term_select() does not have any information about the current taxonomy being required, and the parent function, taxonomy_form(), does not have information about the number of terms. To do this properly, we would need to change the function signature of _taxonomy_term_select(), which is not too bad because it's a private function.
Comment #46
catchLooks like this needs some work.
Comment #47
jaron CreditAttribution: jaron commentedI saw the patch for 5.x, but I'm wondering if there is a version of this fix for 6.13. I have the scenario where there is only one taxonomy term for a given content type and just want it to be selected by default instead of having to actually select from a list of one. I'm a bit of a newbie, so my apologies if it is very simple to convert the patch from 5.x to 6.13. Just don't know how...
(As a side note, I tried using taxonomy_default module but it wouldn't actually load. I'm using TAClite, so maybe there is a conflict?)
Thanks.
Comment #48
bradweikel CreditAttribution: bradweikel commented@jaron - file an issue in the taxonomy_default module describing what went wrong, and I'll work on it there.
All - Despite a fix getting committed to D5, the original problem in this issue reappeared in D6. Haven't checked D7 yet. Will try to take a look tonight.
Comment #49
jaron CreditAttribution: jaron commentedWill do. In the meanwhile, is there a way to patch it (or better that the patch is committed to D6) for D6?
Thanks.
Comment #50
bradweikel CreditAttribution: bradweikel commented@jaron - There is no way this would get patched in D6. At this stage in the cycle, only security issues and other urgent or high demand fixes get in. Definitely no new features.
I'm sure you could patch your own copy relatively easily, or do it with a small custom module, but it sounds like you might not have the PHP background for that so you'll have to find someone to do it for you. My recommendation, though, would be to just use the Taxonomy Defaults module. I'll look at your issue there in the next day or two.
Comment #51
franzBumping
Comment #52
jibranAfter #1847596: Remove Taxonomy term reference field in favor of Entity reference the field part of the taxonomy belongs to entity reference field. Feature requests are part of 8.1.x now.
Comment #53
amateescu CreditAttribution: amateescu commentedThis is the default behavior in Drupal 8 with required Entity reference fields that use the "Select list" widget.
Comment #54
amateescu CreditAttribution: amateescu commentedForgot to change back the component.