Hi,

There is a small bug on required field display.

The * does not appear next to the field and next to a group name if the field is in (I have enabled the option to make group displayed as required if one of its field is required).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

supportPIC’s picture

In the live preview the * is ok, but not in the content editor (not sure why but the #required attributes is empty in the form)

Wim Leers’s picture

Component: Miscellaneous » Code
Assigned: Unassigned » Wim Leers

I will look into this.

acoustika’s picture

Same issue, have a required taxonomy field with depth in a content type...
But i can create node without choosing any value in that field

dafeder’s picture

Same here. Required actually doesn't do anything for hierarchical select fields.

dafeder’s picture

... if I don't require deepest level.

Peter Bex’s picture

There are two problems: one is that the element is constructed in such a way that it replaces the prepopulated fields API configuration (in hook_field_widget_form() in hs_taxonomy.module). This causes it to disregard the "required" attribute. The attached patch simply appends to this array, similar to the code I found in core field elements. Hopefully this doesn't introduce new issues (if it does, a few well-placed unset() calls might help - IMO that's cleaner than just copying over a few selected fields like '#required').

The other problem is the code in form_hierarchical_select_process() that takes what hierarchical_select_process_calculate_return_value() returns and puts it in an array if it isn't an array yet:

  if (!is_array($element['#return_value'])) {
    $element['#return_value'] = array($element['#return_value']);
  }

The reason it doesn't work in the "required" case is that when nothing is selected, hierarchical_select_process_calculate_return_value() returns NULL. NULL is put into an array by this code and this is then seen as a value by hs_taxonomy_widget_process() causing it to think the required value was filled in, passing validation.

I've seen type-fixing code like this a million times, and almost always it is the source of subtle and annoying bugs, so I've tried to rip out this snippet in my patch. Instead, I've ensured that hierarchical_select_process_calculate_return_value() returns a uniform value type: an array.

I'm still worried that this patch may break something else, because the comments for _hierarchical_select_hierarchy_validate() contain a note that $selection is either a single ID or an array. I wasn't able to figure out when it would be a single ID. I think it's better if this is fixed so it's also a uniform type of argument. If that can't be done, fix it so it at least always returns an array.

Mariano’s picture

What's the status of the above patch? will it be included in the next version of the module? thanks!

Wim Leers’s picture

I had already fixed the missing asterisk at #1254104: HS Taxonomy widgets are always optional, #required is ignored. However, your method is clearly superior. I did not even notice the passed in $element. That's the downside of porting working code without solid documentation of Field API, I guess.

Your approach of solving #required support indirectly also fixes #1198362: Use widget label and description for HS Taxonomy field widget is not translated. Yay!

Finally, your #required validation/#return_value calculation does indeed fix the problem. A duplicate of this was reported as the second bug reported at #1242812: Previewing nodes deletes term reference values. Note that this did work just fine in Drupal 6. These bugs always occur in major Drupal version upgrades, due to subtle changes in Forms API (which, I need not tell, is severely underdocumented for advanced use cases such as HS).

Thanks!

The only reason I've not yet committed this, is because I also need to test this in combination with a fix for #1242812: Previewing nodes deletes term reference values.

Wim Leers’s picture

First: #1242812: Previewing nodes deletes term reference values cannot be fixed right away. It's *very* complex.

Second: your patch may not break compatibility with Taxonomy, or really, Field API in general, but it may break compatibility with custom forms. HS does not need to be used to select arrays of values: it may also be used to select a single value. In that case, an array is not necessary, and this is also the case where HS may break compatibility with some modules/forms/use cases, if I'd commit your patch. (Since it no longer normalizes all data to an array for internal use.)

When rewriting HS (HS 4, see http://drupal.org/node/1052670), this will definitely be cleaned up as well :)

Wim Leers’s picture

Title: Required field without * » HS Taxonomy widgets are always optional, #required is ignored: leverage Field API
Status: Active » Fixed

Improved issue title, to show why this is better than #1254104: HS Taxonomy widgets are always optional, #required is ignored.

Another problem was that JS settings would not be resent in recent versions after the page is reloaded

Commits:
- http://drupalcode.org/project/hierarchical_select.git/commit/ebdd678
- http://drupalcode.org/project/hierarchical_select.git/commit/672124e (this one contains my simpler solution to the required checking part of the issue)
- http://drupalcode.org/project/hierarchical_select.git/commit/51bbc1b (the JS settings resending issue)

Peter Bex’s picture

Thanks for your feedback on my patch. It absolutely makes sense to avoid breaking possible custom code; I hadn't considered that.

Wim Leers’s picture

Not just feedback: your patch was also committed, and you're listed as the author: http://drupalcode.org/project/hierarchical_select.git/commit/ebdd678 :)

Peter Bex’s picture

yay! :)

Status: Fixed » Closed (fixed)

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