Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Shevchuk’s picture

Issue summary: View changes
Tobias Xy’s picture

Status: Active » Fixed

That is the expected behavior so far. FCS currently only uses the States API and doesn't validate anything.
But I've thought about adding a server-side validation too... Maybe something like that will be added in a 2.1/2.2 release.

Status: Fixed » Closed (fixed)

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

roball’s picture

Version: 7.x-2.0 » 7.x-2.x-dev
Component: Documentation » Code
Category: Support request » Feature request
Status: Closed (fixed) » Active

Thank you Tobias Xy for thinking of adding this feature. Thus I have changed this issue's category to be a Feature request.

Meanwhile, FCS 2.1. is out. Could we expect this feature in 2.2?

chowdhuriarijit’s picture

Assigned: Unassigned » chowdhuriarijit
chowdhuriarijit’s picture

Status: Active » Needs review
FileSize
1.7 KB

The attached patch will fix the required field validation issue. Now if field conditional state field is set as required field then , it will honor the required field validation.

roball’s picture

Status: Needs review » Needs work

t($conditional['field_name'] . ' field is required') is wrong: You must never put variables, concatenations, constants or other non-literal strings into the t() function. See https://www.drupal.org/node/322732 for details.

chowdhuriarijit’s picture

Status: Needs work » Needs review
FileSize
1.73 KB

@Roball , Ahh its my bad , thanks for the comments :).

roball’s picture

Status: Needs review » Needs work

Thanks for fixing this. Could you please use the same string for this kind of error message as Drupal core uses (in includes/form.inc), thus t('!name field is required.', array('!name' => $conditional['field_name']))? This allows you to have this string translation immediately available for most languages, because it already exists for core. No re-translation is necessary and the format of this message is the same across core and contributed modules.

chowdhuriarijit’s picture

Here is the updated patch.

chowdhuriarijit’s picture

Status: Needs work » Needs review
roball’s picture

Status: Needs review » Needs work

Translatable strings must be 100% identical in order to use the same existing translation. Core and other modules such as Webform are using

!name field is required.

thus this exact string should be reused. Also note the dot at the end. You have now used

!name is required field

which does not match.

chowdhuriarijit’s picture

Status: Needs work » Needs review
FileSize
1.72 KB

Ok changes are done.

roball’s picture

Thank you, now the t() function usage is perfect. That way, it is also supported by my Better Form Errors module out of the box.

kaizerking’s picture

This patch did not work WSOD after applying this pacth

kaizerking’s picture

This patch is not working even after entering the required values this throws error

chowdhuriarijit’s picture

@kaizerking : Provide more details regarding what type of error you are getting. Because i just retested the patch and have not seen any WSOD after applying this on 7.x-2.x-dev. Also the required field checking worked fine .

kaizerking’s picture

I have three fields A, B, C,
A is select list field which has values 1 ,2
If i select 1 in A then B should be visible and required
If i select 2 then A should be visible and required
Here when i select 1 or two the required red star changes as it should be but visible, invisible doesn't work
in the required field if you enter value and save it it wont save. Form throws validation error to enter value

roball’s picture

You have said

This patch did not work WSOD after applying this pacth

but now it sounds like it applied just fine.

chowdhuriarijit’s picture

Kaizerking I am not getting your question, sorry for that. This patch cares only about the required field validation. Visibility thing is not controlled by this patch.

kaizerking’s picture

@chowdhuriarijit, I am sorry for the confusion the WSOD was my mistake while applying the patch manually
After correcting I tested again and cleared the WSOD
I feel even if the patch is for required field validation, IMO It should seamlessly work in any group or individual situations

chowdhuriarijit’s picture

@Kaizerking : Just follow this steps and let me know your results ,
1) create field1 and field2 and make field 2 visible and required based on the value field 1
2) leave field 2 blank and submit the form , you should get form validation error message and that is the expected
behaviour. If any visibility thing which is working before but after applying this patch is not working then it can be a regression , but if you get any visibility issue without applying this patch then that issue should not depended on this , so for that create a separate bug in the issue queue

Thanks

pixelpreview@gmail.com’s picture

Your solution doesn't work for each case
for exemple on my site, I have two checkboxes . When we select a checkbox, I show 2 fields and these fields become required. And this behaviour is for the 2 checkbox, required only if the checkbox is checked... I create a new node, I late the 2 checkboxes unchecked and I have error validation messages ! the 4 hidden fields are not required because the checkboxes are not checked !

stefan.r’s picture

Placeholders should begin with @ or % to prevent XSS attacks, and indeed this should also account for conditionally hidden fields.

chowdhuriarijit’s picture

@pixelpreview : Thanks for the comments , will check the scenario from my end.

pixelpreview@gmail.com’s picture

Don't hesitate to ask me to test your patch @chowdhuriarijit :)

chowdhuriarijit’s picture

Updated patch provided which will fix the issue mentioned by pixelpreview.

brpubs’s picture

This is responding to now-closed https://www.drupal.org/node/2306401.

Thanks so much for the referral to this patch, chowdhuriarijit. I had found and tried it yesterday, though, and ran into problems. It succeeded in enforcing the conditionally required fields -- but it enforced the requirement no matter the state of the field set to control them. Something in my set-up apparently defied the logic of the patch, and I was too fried at that point to try to figure it out.

What did work for me was just to add a custom validation function per http://befused.com/drupal/form-validation. Since I'm not using conditional fields extensively and only needed the conditional validation on a single form, that met my needs well.

chowdhuriarijit’s picture

Status: Needs review » Patch (to be ported)

chowdhuriarijit’s picture

Status: Patch (to be ported) » Closed (fixed)
pixelpreview@gmail.com’s picture

hi,
thanks for the patch but it doesn't work on my install.
My fields are in a field set
I have 3 fields:

  • the first is a check box for the control
  • an image field
  • and a text field

these fields are hidden/show when control field is checked or unchecked.

I don't see warning for mandatory fields when check box is checked. my node is saved.
my states are correct in the settings of the fields

visible : All conditions must be true (AND)
field_name / trigger state : checked
and invisible
field_name / trigger state : unchecked

is it a misconfiguration in my settings ?

pixelpreview@gmail.com’s picture

Status: Closed (fixed) » Active
chowdhuriarijit’s picture

@pixelpreview : will check and let you know.

pixelpreview@gmail.com’s picture

hi @chowdhuriarijit,
the patch doesn't work for me

With these story :
1/ Creation of the controller field (checkbox) == > choice
2/ Creation of 2 no-mandatory fields (image and textarea with ckeditor)
3/ Change the state for these both fields to :

  • required WHEN choice field IS checked
  • no required WHEN choice field IS unchecked

4/ I create a node and I CHECK the choice field and I save. No error messages appear on top of the page, the node is saved.

The patch works for you ?

SantVim’s picture

I have a select field with two options video/audio when video is selected a embedded media field is set as visible and required and when audio is selected another embedded media field is visible and required, The point being only one of the fields needs to be filled when a option is selected.But with this commit am getting a error for both the fields are required even when one has been filled and the other should be empty.

chowdhuriarijit’s picture

@pixelpreview, @Dhamub : Thanks for your input , I will check both of your scenarios and if there will be any fix needed then will do that.

Status: Active » Needs review

Status: Needs review » Needs work
sunfire-design’s picture

Hi,

changing:

if ($conditional['state'] == 'required' && empty($form[$conditional['field_name']][LANGUAGE_NONE][0]['#value']) && !empty($form[$conditional['states'][0]['control_field']][LANGUAGE_NONE]['#value'])) {

in

if ($conditional['state'] == 'required' && empty($form[$conditional['field_name']][LANGUAGE_NONE][0]['value']['#value']) && !empty($form[$conditional['states'][0]['control_field']][LANGUAGE_NONE]['#value'])) {

works for me in dev version

chowdhuriarijit’s picture

Will re check the patch .

Status: Needs work » Needs review

Status: Needs review » Needs work
chowdhuriarijit’s picture

@pixelpreview : I have checked your scenario , i have found issue only for image field . It was working fine for text area with ckeditor field. for image related issues I will provide fix quickly

pixelpreview@gmail.com’s picture

Great :)

chowdhuriarijit’s picture

Updated patch which fixed image field validation issue. Still some improvement need to be done.

chowdhuriarijit’s picture

Status: Needs work » Needs review
FileSize
4.94 KB

patched submitted for testing

zviryatko’s picture

I've added case for Select list field type.

Nikolino’s picture

The patch work fine...
the only thing to add is the class "error" in the field.

roball’s picture

the only thing to add is the class "error" in the field.

So, shouldn't the status be changed to "Needs work" then?

stefan.r’s picture

Status: Needs review » Needs work
jonhattan’s picture

Addressed #50. Attached also an interdiff for clarity.

JugglerX’s picture

I applied the patch from comment #52, but after some testing I am still having problems.

Marco Vervoort’s picture

When I used the patches from comments 27 and 52. I found that the validation-code assumed a very simple conditional rule, namely 'field X is required if field Y is checked'. Unfortunately, this didn't fit my use-case.

I've reworked the code of _field_conditional_state_validate to work for both 'AND' and 'OR' combinations of both 'field Y is checked / not empty' and 'field Y is not checked' conditions. I'm uploading this as a patch replacing the patch from comment 52, to be used together with the patch from comment 27.