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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JirkaRybka’s picture

Yes, 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.

drumm’s picture

Status: Needs review » Active

Needs a patch file attached.

JirkaRybka’s picture

Version: 5.2 » 6.x-dev
Priority: Critical » Normal
Status: Active » Needs review
FileSize
3.23 KB

OK, 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 ;)

webchick’s picture

Status: Needs review » Needs work

Ohhhh 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.

 +  $blank = ($vocabulary->required) ? t('-- Please choose! --') : '<'. t('none') .'>';

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.

JirkaRybka’s picture

Status: Needs work » Needs review
FileSize
3.23 KB

Thanks 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).

mbria’s picture

Thanks 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?

JirkaRybka’s picture

mbria: 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.

mbria’s picture

Sorry 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.

Dries’s picture

+ 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.

JirkaRybka’s picture

FileSize
3.23 KB

Re: 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?).

anantagati’s picture

I am getting PHP notices with taxonomy-required-3.patch when vocabularies are not required on node preview and default values are not selected:

    * notice: Trying to get property of non-object in /var/www/drupal-head/modules/taxonomy/taxonomy.module on line 48.
    * notice: Trying to get property of non-object in /var/www/drupal-head/modules/taxonomy/taxonomy.module on line 49.
    * notice: Trying to get property of non-object in /var/www/drupal-head/modules/taxonomy/taxonomy.module on line 74.
    * notice: Undefined index: in /var/www/drupal-head/modules/taxonomy/taxonomy.module on line 872.
    * notice: Trying to get property of non-object in /var/www/drupal-head/modules/taxonomy/taxonomy.module on line 75.
    * notice: Trying to get property of non-object in /var/www/drupal-head/modules/taxonomy/taxonomy.module on line 75.
    * notice: Trying to get property of non-object in /var/www/drupal-head/modules/taxonomy/taxonomy.module on line 78.
    * notice: Trying to get property of non-object in /var/www/drupal-head/modules/taxonomy/taxonomy.module on line 51.
    * notice: Trying to get property of non-object in /var/www/drupal-head/modules/taxonomy/taxonomy.module on line 444.
    * notice: Trying to get property of non-object in /var/www/drupal-head/modules/taxonomy/taxonomy.module on line 444.
    * notice: Trying to get property of non-object in /var/www/drupal-head/modules/taxonomy/taxonomy.module on line 444.
    * notice: Trying to get property of non-object in /var/www/drupal-head/modules/taxonomy/taxonomy.module on line 444.
anantagati’s picture

Same problem during node preview for required vocabularies.

anantagati’s picture

FileSize
1.61 KB

I 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.

catch’s picture

devadarshan: 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.

anantagati’s picture

Oh sorry, I see, problem is in HEAD version.

JirkaRybka’s picture

Re: #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...

catch’s picture

property of a non-object: http://drupal.org/node/186837

undefined index: http://drupal.org/node/186750

JirkaRybka’s picture

FileSize
1.37 KB

OK, 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.

JirkaRybka’s picture

The patch still applies cleanly. And quite literally - needs review! ;)

catch’s picture

Status: Needs review » Reviewed & tested by the community

Applies 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.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thank, committed.

webchick’s picture

Version: 6.x-dev » 5.x-dev
Status: Fixed » Patch (to be ported)

Any way this would be considered for 5.x?

webpotato’s picture

I'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?

JirkaRybka’s picture

The 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

JirkaRybka’s picture

FileSize
1.38 KB

Believe 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.

JirkaRybka’s picture

Status: Patch (to be ported) » Needs review
drumm’s picture

Status: Needs review » Fixed

Committed to 5.x.

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

bfsworks’s picture

Priority: Normal » Critical
Status: Closed (fixed) » Active

BIG 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.

JirkaRybka’s picture

Seems 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.

bfsworks’s picture

I 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.

JirkaRybka’s picture

FileSize
488 bytes

I 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.)

ju.ri’s picture

i 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?

JirkaRybka’s picture

Did 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).

catch’s picture

Looks like a related fix (RTBC) here for views: http://drupal.org/node/199675

ju.ri’s picture

i did indeed forget to flush the views cache. thanks! i will open an issue in views project now.

drumm’s picture

Status: Active » Fixed

Reverting status since this views issue is being handled elsewhere.

Summit’s picture

Hi,

I have somewhat the same thing on hand I think.
I got the following message:

Taxonomy: Term must have a value! 

But I want to use the taxonomy term as filter, and non of the vocabularies is required...
Please assist.
Thanks in advance,
Greetings,
Martijn

francoud’s picture

I 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... :(

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

Summit’s picture

Status: Closed (fixed) » Active

Hi,

This fixed it for me: http://drupal.org/user/58871
But I think the fix is not committed yet, right?

greetings,
Martijn

webchick’s picture

Status: Active » Closed (fixed)

If 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.

longwave’s picture

Priority: Critical » Normal
Status: Closed (fixed) » Needs review
FileSize
546 bytes

I 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)); to unset($options['']);

chx’s picture

Title: First taxonomy term selected by default » Only taxonomy term selected by default when there is only one
Version: 5.x-dev » 7.x-dev
Category: bug » feature
floretan’s picture

We 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.

catch’s picture

Status: Needs review » Needs work

Looks like this needs some work.

jaron’s picture

Version: 7.x-dev » 6.13

I 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.

bradweikel’s picture

Version: 6.13 » 7.x-dev

@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.

jaron’s picture

Will do. In the meanwhile, is there a way to patch it (or better that the patch is committed to D6) for D6?

Thanks.

bradweikel’s picture

@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.

franz’s picture

Version: 7.x-dev » 8.x-dev

Bumping

jibran’s picture

Version: 8.0.x-dev » 8.1.x-dev
Component: taxonomy.module » entity_reference.module
Issue summary: View changes

After #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.

amateescu’s picture

Version: 8.1.x-dev » 7.x-dev

This is the default behavior in Drupal 8 with required Entity reference fields that use the "Select list" widget.

amateescu’s picture

Component: entity_reference.module » taxonomy.module

Forgot to change back the component.

  • Gábor Hojtsy committed e0c7c0e on 8.3.x
    #180109 by JirkaRybka: overcome browser quirk to detect when no taxonomy...

  • Gábor Hojtsy committed e0c7c0e on 8.3.x
    #180109 by JirkaRybka: overcome browser quirk to detect when no taxonomy...

  • Gábor Hojtsy committed e0c7c0e on 8.4.x
    #180109 by JirkaRybka: overcome browser quirk to detect when no taxonomy...

  • Gábor Hojtsy committed e0c7c0e on 8.4.x
    #180109 by JirkaRybka: overcome browser quirk to detect when no taxonomy...

  • Gábor Hojtsy committed e0c7c0e on 9.1.x
    #180109 by JirkaRybka: overcome browser quirk to detect when no taxonomy...