I have a multi-page Webform. I have installed IFE and set-up as per the instructions. IFE doesn't work on some of the fields in my webform. It appears that it doesn't work with Grid fields or Time fields at all. It also doesn't work with radio fields that use the Select or Other module. If it should work with these fields then maybe I'm missing something and any help would be gratefully received.

F

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

stijndm’s picture

Status: Active » Needs work

Had a quick look at this but can't get it to work for the moment. The module picks up the errors, but everything stops when it needs to be rendered.

JoseTvm’s picture

xamanu’s picture

Title: WebForm Validation Errors not displaying in-line » Missing support for complexer field rendering (radios, date, etc...)
Version: 6.x-1.1 » 7.x-2.x-dev
Priority: Normal » Major
Status: Needs work » Needs review
FileSize
3.49 KB

I still have this for the 7.x version. Not working fields (and there should be a lot more): radios, date, email.
Since radio buttons are even core fields I go up with the priority. Digging into this I realized that the recursive function call in "ife_element_errors_set()" is not even close to be the right logic. I attach a patch which is at least working but it would be worth to think and discuss this a little but to get a real good solution.

I think this is somewhat the heart of the module...

pacproduct’s picture

I'm facing the same problem and patch #3 solved the handling of those elements.
However, in my case in does not pick-up elements like "title" because of the following piece of code that the patch contains:
if (strstr($error_anchor, $field_name . ']', true) === '') {

Because of the . ']', the 'title' field does not get picked-up. Do we need that?
If I remove that concatenation, it is working fine for me.

pacproduct’s picture

Attached to this post: Xamanu's patch (#3) with my suggested change in #4.
This would require some tests/reviews from the community though.

michaellenahan’s picture

The patches at #3 and #5 caused inline field errors on webforms to disappear entirely, for me anyway.

webform 7.x-3.18
ife 7.x-2.0-alpha2

I solved the "no inline errors for radio buttons" problem with the attached patch.

I suspect this isn't a complete solution, but it works so far for me :)

Dean Reilly’s picture

Status: Needs review » Needs work
FileSize
877 bytes
+++ b/ife.module
@@ -258,39 +258,71 @@ function ife_errors($op = 'get', $id = NULL, $message = NULL) {
+  // Check for complexer fields, used later for proper rendering
+  $field_types_complex = array('radios' => FALSE,);
+  drupal_alter('ife_complex_types' , $field_types_complex);

This needs a lot more documentation to let the user know about this hook and what it's for.

We should also explain why we set radios to false. It's not immediately obvious at all. And the trailling comma is in violation of Drupal's coding standards.

I'd also change this from a drupal_alter toa module_invoke and move our definition to our own ife hook.

Finally $complex_field_types probably reads better.

+++ b/ife.module
@@ -258,39 +258,71 @@ function ife_errors($op = 'get', $id = NULL, $message = NULL) {
+  // Go through all children of form
+  foreach (element_children($form) as $field_name) {

Now that we've got rid of the recurssion we're not going to be able to deal with deep hierarchies. Take a look at the parents recursion test form in the module I've attached.

+++ b/ife.module
@@ -258,39 +258,71 @@ function ife_errors($op = 'get', $id = NULL, $message = NULL) {
+      // Compare if error for given field has been set.
+      if (strstr($error_anchor, $field_name, true) === '') {

This confuses the form #parents property (which is how we identify form errors) with the form array structure.

https://drupal.org/node/48643

Again see the parent example for where this fails.

strstr is also very slow. If you're checking for the existence of a string in a known location use substr to get that location and compare the result.

+++ b/ife.module
@@ -258,39 +258,71 @@ function ife_errors($op = 'get', $id = NULL, $message = NULL) {
+          if (count($_SESSION['messages']['error']) <= 0) {

Count can't return less than 0 so let's not bother checking that case.

http://php.net/count

+++ b/ife.module
@@ -258,39 +258,71 @@ function ife_errors($op = 'get', $id = NULL, $message = NULL) {
+          // Extract the latest element of field.

latest -> last

+++ b/ife.module
@@ -258,39 +258,71 @@ function ife_errors($op = 'get', $id = NULL, $message = NULL) {
+          while ($element_child = element_children($element)) {
+            $element_child = array_pop($element_child);
+            $element = $element[$element_child];

This is only ever going to get the last child element from a group. Take a look at the tree recursion example for where this fails.

+++ b/ife.module
@@ -258,39 +258,71 @@ function ife_errors($op = 'get', $id = NULL, $message = NULL) {
+            // Check if a complexer type of field is used here

complexer -> more complex

+++ b/ife.module
@@ -258,39 +258,71 @@ function ife_errors($op = 'get', $id = NULL, $message = NULL) {
+              if (function_exists($field_types_complex[$element['#type']])) {

Let's not be defensive. We should probably just try and call the function directly. That way if I do make an error in declaring my identification function I'll get an error rather than it failing silently.

Dean Reilly’s picture

Title: Missing support for complexer field rendering (radios, date, etc...) » Nested fields don't render errors correctly
Status: Needs work » Needs review
FileSize
3.13 KB

Here's another way to go about it which I think works a little better.

Dean Reilly’s picture

+++ b/ife.module
@@ -258,39 +258,50 @@ function ife_errors($op = 'get', $id = NULL, $message = NULL) {
+      ife_element_errors_set($elements[$key], $elements, FALSE);

Second argument should be $display.

Dean Reilly’s picture

So I've had some time to think about this more. What the lsat patch basically does is a depth first traversal of the form tree looking for elements with an error. Once it finds one it stops searching that branch. We stop because Drupal will treat each of the child elements as belonging to that same error and we don't want to repeat a load of inline errors.

This might cause problems though where a child of the current branch has a different error message. Are search will never see that node so the different error message won't be printed. What we need to do is continue the search down the child elements but ignore errors which have already been raised against their parents. This patch adopts this solution.

pacproduct also mentioned to me that my patch doesn't work with the date field. That's because date raises the error against a container. The function associates the error with the container element but we never alter the element info because it's not an input element so we don't output it. I'm not sure why only alter certain elements, seems like it would be better to alter everything and push responsibility for raising an error against the correct form elements back on to the implementing modules.

After I made that fix it still didn't work but that's because date actually removes our added theme_wrapper function. That seems to be a bug in the date module so we should have them fix it rather than trying to work around it.

Dean Reilly’s picture

I've opened an issue with the Date module here #2068825: Date form elements clobber #theme_wrappers to deal with the specific problem I mentioned in #10.

stijndm’s picture

Issue summary: View changes
Status: Needs review » Fixed

This should now be fixed in the latest 2.x dev branch. A new release is coming, in the meantime, would you mind trying out the dev release?

Dean Reilly’s picture

Status: Fixed » Needs work

Hey stijndm,

I assume this is the commit you made to address this issue:

http://cgit.drupalcode.org/ife/commit/?id=46cf046&context=40&ignorews=0&...

function ife_element_errors_set(&$element, $display, $carry_down = FALSE) {

What's the purpose of the new $carry_down argument on ife_element_errors_set()? It doesn't seem to do anything.

	// Found a matching error, no need to continue.
	return;

If you return at this point you risk missing different errors on nested elements.

stijndm’s picture

Hi Dean,

Thanks for the review. Good catch on the $carry_down. I was experimenting and trying different solutions. This one is a leftover and shouldn't be there.
I was specifically having trouble with wrapped elements where the error was triggered on the wrapper instead of the element causing either no error to show, the error to show up in the wrong place or the error to show up more than once.

I eventually settled for the solution in the commit since this seems to catch all errors and adds them on the fly to the correct element and prevents the problems I was facing. This worked for my basic tests with the most common elements.

The return was added for wrapped or grouped elements. Such as a set of radio buttons. Say you have an element with two radio buttons, you would get three error. On for with the label of the wrapped element, and one for each radio button.

This didn't seem to affect other grouped or wrapped elements (such as a date dropdown) in a negative way but I agree further testing is required.

MrHaroldA’s picture

stijndm: "A new release is coming" dating back to May 19, 2014 ;)

Anyway; I ran into a problem with date elements in the Webform module: #2508267: Place form errors on the date element wrapper, not the individual date form elements

Webform places error messages on individual elements in a date element. This renders quite ugly with IFE enabled, but it's the preferred way to add messages in the Form API, as the maintainer of Webform said.

The -dev version of IFE seems to destroy Webforms with page breaks by removing all form elements when there are errors present, and the git commit does not apply on Alpha 2. I guess IFE isn't in a great state these days?

What can we do to get IFE up-and-running again?

deanflory’s picture

I've checked back with IFE a few times in the last few years and there was always some error that kept me from using it so I wouldn't hold your breath.