CRM-18427: Submitted custom data values not reloaded on form when validation fails #8269

monishdeb · 2016-04-29T11:16:14Z

CRM-18427: Submitted custom data values not reloaded on form when validation fails

monishdeb · 2016-04-29T12:07:31Z

Jenkin, test this please

monishdeb · 2016-04-29T12:17:49Z

@colemanw @eileenmcnaughton as per https://issues.civicrm.org/jira/browse/CRM-16676?focusedCommentId=86474&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-86474 reason behind this bug, this is the only fix I got, as now via 'civicrm/custom?type=&' loads CRM_Custom_Form_CustomDataByType snippet which only got info what we provide via url's parameters. This fix contain -

json_encode([submitted custom values]) and append to the link
https://github.com/civicrm/civicrm-core/pull/8269/files#diff-8dd71cd2b4ff3bc91e82e75dcdf32badR58
When page reloads .. decode the data and use for setting default value
https://github.com/civicrm/civicrm-core/pull/8269/files#diff-d9f1be3fcfd7cea0a4864a21997c258aR1862
Though it looks a bit hackish so I have marked my PR with WIP if there is any scope to improve my PR !!

colemanw · 2016-05-14T20:20:59Z

jenkins, retest this please

monishdeb · 2016-05-17T11:45:33Z

Test build failure is not related

eileenmcnaughton · 2016-05-20T05:18:38Z

CRM/Core/BAO/CustomGroup.php

@@ -1377,7 +1377,13 @@ public static function setDefaults(&$groupTree, &$defaults, $viewMode = FALSE, $
                }
              }
              else {
-                $checkedValue = explode(CRM_Core_DAO::VALUE_SEPARATOR, substr($value, 1, -1));
+                if (is_array($value) && count($value)) {


I have tested this patch by adding a new contribution & filling it in with contribution details & custom field details but no amount. I save the custom data disappears. However, with this patch it does not.

I'm just going through the patch to see if it makes sense / seems risk free.

This line seems risk free as an array would not have previously been handled & the non array path is unchanged

I tested radio, checkbox, autocomplete, date, multiselect, select, adv multiselect..

eileenmcnaughton · 2016-05-20T09:15:44Z

I think this is OK - it works & as far as I can see by getting the values only for specific fields (custom_xx) it's safe & being in the Custom class it should be fairly hard for someone to foolishly extend it to put credit card fields into the url.

Having said that, odd interaction with the $_POST array makes me inherently a little nervous so I'd really like @colemanw or @totten or @xurizaemon to quickly eyeball this & confirm they don't see any hole that I'm missing

monishdeb · 2016-05-24T04:49:31Z

Jenkin test this please

xurizaemon · 2016-06-06T04:57:00Z

I'm not comfortable with this ... can't articulate very well right now, but -

$_FILE uploads won't work with being translated to $_GET
potentially sensitive personal data (custom data) shouldn't be moved to $_GET params
$_GET won't work with larger input fields (text)

So using $_GET feels like an incomplete/not correct fix to me.

monishdeb · 2016-06-06T11:00:40Z

@xurizaemon I have a plan to fix that. How about we capture the same logic to add/delete attachments in activity, for custom file field too ? So now when user uploads a file then on submit, it actually made an entry in civicrm_file and other necessary table, so when the page reloads on validation error, it fetch the saved file entry (show as a link against the custom file field) BUT now you have a trash icon against the file link so you can delete and upload a new file.

#2 and #3 I agree thats it not right to pass such personal data and its weird to pass large input data via $_GET. So I was thinking what if we use cache to store/fetch this submitted values and later used to set default custom values on form reload!!

@eileenmcnaughton @colemanw is it ok ?

xurizaemon · 2016-06-06T15:37:33Z

@monishdeb do you understand why these form fields need to be treated "specially"? I don't yet get the underlying cause of the need for this workaround. (Sorry, I know I'm chiming in late here.)

eileenmcnaughton · 2016-06-22T00:57:50Z

@colemanw I feel like you need to look at this as you fixed something similar on the payment form

colemanw · 2016-07-19T23:59:16Z

@monishdeb I agree that the cache option is a good one. Using $_POST instead of $_GET also seems like an acceptable solution.

…idation fails

monishdeb · 2016-07-22T14:32:19Z

@colemanw @eileenmcnaughton I have updated the PR and this is my change ac8833f as now we are storing the custom data in cache rather the passing it to url. As you can see I used to CRM_Core_BAO_Cache::setItem()/getItem() instead of Civi::cache->set()/get() because

It un-/serialize submitted data on get/set respectively
Always update the cache against corresponding cache path

However $_FILE uploads doesn't work not only for custom file field but also for other entity files e.g. activity attachments. So it should be raised as a separate issue.

colemanw · 2016-07-22T15:27:36Z

CRM/Core/BAO/CustomGroup.php

-    }
+    // Generate cache key against which submitted custom value will be stored
+    $cacheKey = implode('_',
+      array(__CLASS__,


__CLASS__ will always be the same e.g. for forms open in multiple tabs. Do we have access to qfKey here?

Ya I tried with qfKey earlier but getting some trouble with it as formatGroupTree() function is being called by multiple form as in here 'CRM_Custom_Form_CustomDataByType' form which is called via loadPage and entity form. So qfkey keeps changing for this two form instance. I tried to use get_class($form) != 'CRM_Custom_Form_CustomDataByType' as another flag to set/get qfkey but it throws invalid session handling error on page reload. Still lemme check if we can make use of qfkey.

monishdeb · 2016-07-22T19:40:37Z

Done, updated the PR with f22fdc2 . Used qfKey only to be use a cache key.

colemanw · 2016-07-22T21:25:30Z

CRM/Core/BAO/CustomGroup.php

@@ -1892,7 +1894,9 @@ public static function formatGroupTree(&$groupTree, $groupCount = 1, &$form = NU

    if ($form) {
      if (count($formValues)) {
-        $form->assign('submittedValues', json_encode($formValues));
+        $qf = $form->get('qfKey');
+        $form->assign('qf', $qf);


'qfKey' seems like a better name for this smarty variable.

josephlacey · 2016-07-22T23:21:13Z

Thanks for all the work on this, y'all . I can test whenever the patch is ready, so just let me know.

However $_FILE uploads doesn't work not only for custom file field but also for other entity files e.g. activity attachments. So it should me raised as a separate issue.

I'd say there's a low expectation that this needs to work. A fair number of web applications I use don't save the upload file value when you fail validation.

colemanw · 2016-07-23T00:29:49Z

@josephlacey feel free to test now; my last suggestion was a NFC.

monishdeb · 2016-07-24T17:48:27Z

@colemanw done (but can't rename the url param to qfKey as it overrides the qfkey of corresponding dataUrl's form and eventually ends up into valid session key error on page reload)

monishdeb · 2016-07-24T17:48:53Z

PR is ready for test :)

jitendrapurohit · 2016-08-02T06:00:21Z

Tested the PR with different input custom fields, changes works fine for all of them.

monishdeb · 2016-08-02T07:33:20Z

@josephlacey I am merging this PR as Jitendra had already tested for all kind of custom fields except file. Feel free to reopen the ticket if ya find any other bug.

monishdeb added needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state 4.7 labels Apr 29, 2016

civicrm-builder added the master label Apr 29, 2016

monishdeb force-pushed the CRM-18427 branch from f0306cd to a96d070 Compare April 29, 2016 11:22

totten mentioned this pull request May 6, 2016

Invitation: CiviCRM Release for June 2016 civicrm/release-management#2

Closed

monishdeb removed the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label May 6, 2016

eileenmcnaughton reviewed May 20, 2016
View reviewed changes

JoeMurray mentioned this pull request Jul 7, 2016

Invitation: CiviCRM 4.7.10 release August 2016 civicrm/release-management#4

Closed

CRM-18427: Submitted custom data values not reloaded on form when val…

fb56e96

…idation fails

monishdeb force-pushed the CRM-18427 branch from a96d070 to ac8833f Compare July 22, 2016 14:18

colemanw reviewed Jul 22, 2016
View reviewed changes

monishdeb force-pushed the CRM-18427 branch from ac8833f to f22fdc2 Compare July 22, 2016 19:36

colemanw reviewed Jul 22, 2016
View reviewed changes

store/fetch the submitted data to/form cache

29a1df5

monishdeb force-pushed the CRM-18427 branch from f22fdc2 to 29a1df5 Compare July 24, 2016 17:44

monishdeb merged commit dbfd6b9 into civicrm:master Aug 2, 2016

monishdeb deleted the CRM-18427 branch August 2, 2016 07:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-18427: Submitted custom data values not reloaded on form when validation fails #8269

CRM-18427: Submitted custom data values not reloaded on form when validation fails #8269

monishdeb commented Apr 29, 2016 •

edited by civicrm-builder

Loading

monishdeb commented Apr 29, 2016

monishdeb commented Apr 29, 2016 •

edited

Loading

colemanw commented May 14, 2016

monishdeb commented May 17, 2016

eileenmcnaughton May 20, 2016

eileenmcnaughton May 20, 2016

eileenmcnaughton commented May 20, 2016

monishdeb commented May 24, 2016

xurizaemon commented Jun 6, 2016

monishdeb commented Jun 6, 2016 •

edited

Loading

xurizaemon commented Jun 6, 2016

eileenmcnaughton commented Jun 22, 2016

colemanw commented Jul 19, 2016

monishdeb commented Jul 22, 2016 •

edited

Loading

colemanw Jul 22, 2016

monishdeb Jul 22, 2016 •

edited

Loading

monishdeb commented Jul 22, 2016

colemanw Jul 22, 2016

josephlacey commented Jul 22, 2016

colemanw commented Jul 23, 2016

monishdeb commented Jul 24, 2016

monishdeb commented Jul 24, 2016

jitendrapurohit commented Aug 2, 2016 •

edited

Loading

monishdeb commented Aug 2, 2016

CRM-18427: Submitted custom data values not reloaded on form when validation fails #8269

CRM-18427: Submitted custom data values not reloaded on form when validation fails #8269

Conversation

monishdeb commented Apr 29, 2016 • edited by civicrm-builder Loading

monishdeb commented Apr 29, 2016

monishdeb commented Apr 29, 2016 • edited Loading

colemanw commented May 14, 2016

monishdeb commented May 17, 2016

eileenmcnaughton May 20, 2016

Choose a reason for hiding this comment

eileenmcnaughton May 20, 2016

Choose a reason for hiding this comment

eileenmcnaughton commented May 20, 2016

monishdeb commented May 24, 2016

xurizaemon commented Jun 6, 2016

monishdeb commented Jun 6, 2016 • edited Loading

xurizaemon commented Jun 6, 2016

eileenmcnaughton commented Jun 22, 2016

colemanw commented Jul 19, 2016

monishdeb commented Jul 22, 2016 • edited Loading

colemanw Jul 22, 2016

Choose a reason for hiding this comment

monishdeb Jul 22, 2016 • edited Loading

Choose a reason for hiding this comment

monishdeb commented Jul 22, 2016

colemanw Jul 22, 2016

Choose a reason for hiding this comment

josephlacey commented Jul 22, 2016

colemanw commented Jul 23, 2016

monishdeb commented Jul 24, 2016

monishdeb commented Jul 24, 2016

jitendrapurohit commented Aug 2, 2016 • edited Loading

monishdeb commented Aug 2, 2016

monishdeb commented Apr 29, 2016 •

edited by civicrm-builder

Loading

monishdeb commented Apr 29, 2016 •

edited

Loading

monishdeb commented Jun 6, 2016 •

edited

Loading

monishdeb commented Jul 22, 2016 •

edited

Loading

monishdeb Jul 22, 2016 •

edited

Loading

jitendrapurohit commented Aug 2, 2016 •

edited

Loading