Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: required extrafields: bad check to see if empty + prevent letting it empty (Issue #11169) #11202

Closed
wants to merge 2 commits into from
Closed

FIX: required extrafields: bad check to see if empty + prevent letting it empty (Issue #11169) #11202

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d351a96
FIX: required extrafields: bad check to see if empty + prevent lettin…
May 18, 2019
9d568c9
FIX: required extrafields: revert to old empty check
May 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions htdocs/core/class/extrafields.class.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1057,7 +1057,7 @@ function showInputField($key, $value, $moreparam='', $keysuffix='', $keyprefix='
}

$out.='<select class="flat '.$morecss.' maxwidthonsmartphone" name="'.$keyprefix.$key.$keysuffix.'" id="'.$keyprefix.$key.$keysuffix.'" '.($moreparam?$moreparam:'').'>';
$out.='<option value="0">&nbsp;</option>';
if(empty($required)) $out.='<option value="0">&nbsp;</option>';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you remove this line when field is required, it means the combo list will be automatically set to a default value (the first one). The record will be saved with a value that was not decided/defined explicitely by user. Why setting the default value on first value and not last one.
When a field is mandatory, we want the user to enter it and if we automatically set it to a random value (first one is same than random), we make the oposite of what we want. The check is done later (except if bug) after submitting. We can also avoid empty value by adding a required HTML 5 flag may be.

foreach ($param['options'] as $key => $val)
{
if ((string) $key == '') continue;
Expand Down Expand Up @@ -1829,7 +1829,7 @@ function showSeparator($key, $object)
function setOptionalsFromPost($extralabels, &$object, $onlykey='')
{
global $_POST, $langs;
$nofillrequired='';// For error when required field left blank
$nofillrequired=0;// For error when required field left blank
$error_field_required = array();

if (is_array($this->attributes[$object->table_element]['label'])) $extralabels=$this->attributes[$object->table_element]['label'];
Expand Down Expand Up @@ -1860,8 +1860,7 @@ function setOptionalsFromPost($extralabels, &$object, $onlykey='')
if ($this->attributes[$object->table_element]['required'][$key]) // Value is required
{
// Check if empty without using GETPOST, value can be alpha, int, array, etc...
if ((! is_array($_POST["options_".$key]) && empty($_POST["options_".$key]) && $_POST["options_".$key] != '0')
|| (is_array($_POST["options_".$key]) && empty($_POST["options_".$key])))
if (empty($_POST["options_".$key]))
Copy link
Member

@eldy eldy May 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If value is '0', empty will be true, so it will return an error.
For a combo list with multiple choice with first choice that has key '0' and label "My value 0" will not be possible to be selected. Old code seems good.

Copy link
Contributor Author

@ATM-Marc ATM-Marc May 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted to the old check but I don't really understand your use case. The tooltips for checkbox- and select-typed extrafields say that the keys should not be '0', so that Dolibarr itself could propose the empty choice with '0' key and empty label. Is the tooltip incorrect ?

Still, making the '0' choice available if the field is required seems an issue to me.

Copy link
Member

@eldy eldy May 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "tooltips" has right for combo.
But the test should works also for string or value. And for string '0' is not empty.
I pushed a better fix...

{
//print 'ccc'.$value.'-'.$this->attributes[$object->table_element]['required'][$key];
$nofillrequired++;
Expand Down