Components with a value that are hidden by conditionals should be empty

Any update on this? Does this still happen, and is it related strictly to drafts?

Hmmmm. I agree that it is not totally clear what should happen here. I'm going to mark this as postponed until there is come consensus. I could see:

1. Leaving this as it is. The data was entered and was hidden (but not removed). Defensible, I think. There might be use cases where this would be desirable. For example, someone enters a bunch of data, but then hides it. Changes their mind. Poof! Cool, the data is back.
2. Remove the data each time the submission is saved. I think this is probably a bad idea. If you save a draft (or autosave) in the wrong state, your data is lost.
3. Remove the data only when a valid, complete submission is saved.
4. Option on a per node basis. Don't like this one much.

I looked into this a bit further. The situation is not as discussed in #3. Values which are hidden by conditions are always intended to be empty (i.e. not stored in database). In the particular situation originally described, what happened was:
1) The draft was saved with a value on page 2.
2) The user returned to page one and changed a value which would hide the value.
3) Upon returning to page 2, the field was not sent to the browser at all.
4) When the form is submitted, the value is left in the database.

One possibility would be when a form is saved (submitted, auto-save-on-prev/next, auto-save-on-validate) to recursively find any data in the database that should be hidden based upon source components on the page and delete them. In this example, when the user returned to the first page, changed the value, and then left again, that would trigger the data to be deleted.

A simpler test would be to send components that are completely hidden from the page to the browser anyhow, as empty hidden HTML. This would cause the data to be deleted later when submitted. This would not "fix" data that was hidden by backward reference -- for example, the component on page 2 hides the item on page 1. This is a rather degenerate case and is perhaps not worth supporting.

I am expanding the scope of this issue.

When conditionals are evaluated by jQuery, hidden components are not treated as empty, but they are (I think) when evaluated by the server. For example

Text field = 'hi' hides Number field
Number field = 1 hides Date field

Put 1 in Number field to hide date field, then put 'hi' into text field to hide number field. The date field should reappear since it should be considered empty.

Now imagine that Number field resides in a page prior to Text and Date. You would ideally like the same behavior.

We should perhaps put all dependent (recursively) components into hidden fields and send them to the browser. This would solve the problem of removing hidden data on other pages (including previous pages), as well as create DOM elements to trigger jQuery.

The good news is that hiding is equivalent to changing the data to empty, we are laying the groundwork for conditionals to set values.

A possible optimization:

If a node in the dependency graph plus all of its descendants are all for components that reside on a page AFTER the current page, then they don't need to be sent to the browser. However, this also means that when webform skips over entirely blank pages, it needs to remove the data on those pages from the database or form_state['storage'] because they would never be sent to the browser.

Boy, conditionals and multiple pages interact in unfortunate ways.

Let's step back from the abyss. I don't see a reason to support backward conditionals -- conditionals that change a component on a previous page. This simplifies things quite a bit, but we have to add a warning for this condition.

Updated issue summary.

A hideously large, but cohesive patch.

Theory of Operation
When a webform node is loaded, a WebformConditionals object is added to the node as $node->webform[conditional_sorter]. The sorter initially does nothing, but when called upon, analyzes the conditionals to create a topological sort order, which provides the correct order of execution of the conditionals. This sort order is grouped by page. Additionally, a page map is created to help determine which page a particular component is on, and a list of errors found. All data is cached. When the webform_client_form is cached, so is the sorter, avoid the need to re-analyze the dependencies for each page of a multi-page form.

The sorter can also be used to evaluate the conditionals on a given submission to ensure that the server is making the same decisions as the browser. This also provides conditional operation in the absence of JavaScript.

webform.conditionals.inc

1. When the conditionals form (used to define the conditionals) is presented, the existing conditionals are run through the sorter and any resulting errors displayed. This replaces the existing test for having more than one conditionals target the same components as this is detected by the sorter. There are additional error messages for forward references (references to a page that hasn't been presented yet) and circular references within a page.
2. The jQuery settings containing the conditionals was changed to be sent to the browser in topological sort order. Additionally, only the conditionals that are actually present on the given page are sent. Accordingly, the function signature to webform_conditional_prepare_javascript() was changed to add the page number. This is a slight API change in the off-chance another module is calling the function (which is really intended to be internal).
3. The code in the above function was cleaned up a bit.

webform.webformconditionals.inc
This file contains the WebformConditionals class and nothing else. The methods are:

1. factory(). A static method to create a singleton-for-each-node.
2. constructor(). Trivial.
3. topologicalSort(). Function to actually perform the sort. Also generates the errors and the page map. The algorithm is described in wikipedia: http://en.wikipedia.org/wiki/Topological_sorting
4. Of note is that the algorithm is run repeatedly for each page, and also takes the implied dependencies of fieldsets and pagebreaks into consideration.
5. getOrder(). Returns the conditionals index by page number and execution order.
6. reportErrors(). Calls drupal_set_message with either the errors (if the user has access to the webform settings) or a generic end-user message that the webform needs love.
7. getChildrenMap(). Used to execute the conditionals. Cached. Actually this data is created when the conditionals are loaded but then removed. This duplication of effort should be addressed once this patch is committed.
8. deleteFamily(). This sets any data that is hidden to NULL.
9. execute(). This executes the conditionals on a given submission, or a given page number (or all pages)

webform.js
The JQuery that runs was changes, removing the re-triggering logic added in the related issue and replacing it with execution of all the conditionals in topological order. This includes the initial execution that happens upon page load. The DOM is no longer used for retriggering.

webform.info
New class was added.

webform.module

1. The sorter is added to $node->webform at node_load-time.
2. Errors in the webform conditionals are reported upon display of the client form.
3. The conditionals are executed on the input values to ensure that previous values are consistent and that hidden values from the browser are removed.
4. The form submission code was simplified in how it merges in stored values. Additionally, the conditionals are executed to ensure that hidden fields are removed.
5. When the data is being prepared to be saved for an existing submission, database data from the pages up through the current page is removed before being merged with the submitted data. The prevents old values from the database for fields that are being hidden from being re-inserted into the submission.
6. JavaScript settings are not sent to the preview page.

Additional possible tasks.

1. There is a lot of duplication between the sorter->execute() and the webform_conditionals_rule_check function().
2. It may well be better to store empty submission values as not present, rather than empty strings.
3. There is duplication of effort in creating the component tree and creating the child map needed for execution.
4. If the component weight was guaranteed to be in component tree traversal order, it would be much easier to create the client form.

This is a large, fairly complicated patch that needs solid review.

Test fails because the testbot's class registry is stale. Failure is unrelated to this patch. I'm not sure if there is a way to change the status to Needs Review without starting the testbot.

My error. Patch was missing the new file. I still think testbot's class registry is going to be stale though.

I may remove the singleton code from the factory. I don't think it's currently used because whenever I needed the sorter, I already had the loaded node.

Thanks for taking this on Dan. This is going to take a while for me to totally understand what's happening inside the new class. I can only comment on the general approach to implementation at first pass. A few thoughts:

- The new WebformConditionals class is essentially a "controller", in that it generally does a lot of actions rather than being a data-store of some kind. I'm generally not a fan of controller-like classes in the first place, but in situations where it can help with clarity (such as our existing Exporters) I try not to begrudge them too much. If it's possible to simplify this in a procedural approach, I'd find that preferable.

- Drupal itself already contains an acyclic graph search function (in includes/graph.inc, it's used to ensure dependencies are met when installing/uninstalling modules). It's quite a bit less comprehensive than this class, but could it be used to simplify our approach?

- If the WebformConditionals class is indeed a controller, it shouldn't be bundled into the $node object. The $node object should be a container of data, not objects that perform actions on itself. I'd prefer to see something like a webform_get_conditional_sorter($node) function that statically stored these classes if they're going to be used repeatedly.

So instead of this:

if (empty($form_state['triggering_element']['#ajax'])) {
  $node->webform['conditionals'] = $conditionals;
  $node->webform['conditional_sorter']->reportErrors($conditionals);
}

We would get the sorter separately:

if (empty($form_state['triggering_element']['#ajax'])) {
  $node->webform['conditionals'] = $conditionals;
  $sorter = webform_get_conditional_sorter($node);
  $sorter->reportErrors($conditionals);
}

That would keep our "controller" (the sorter) functionality separate from our data (the $node). Importantly for Drupal users, it wouldn't pollute the $node data with a bunch of internal, temporary, and recursive information when trying to print_r() or debug() the $node variable.

- Let's please rename "execute" method using a verb that describes what it's doing. Without reading the docblock, I would have no idea what the execution of a "WebformConditional" would do. Execute reminds me of the Kingdom of Nouns.

Wut, wait, the patch passed testbot! I wasn't expecting it to be able to cope with the stale registry. Cool.

Thanks for the excellent comments, quicksketch.

- I think this is stronger as a class, especially since its methods share data and the protected methods guarantee that the data isn't written to or accessed when it hasn't been calculated yet. Because of the convoluted way forms get loaded, it is beneficial to put the controller in the $node. This avoids having to recalculate the topological order on every next/prev/save draft page transition because $form['#node'] is cached. This is rather unlike an entity controller where you explicitly want a fresh instance. I very much want to reuse the already-calculated object.

- I actually didn't know about graph.inc, but can reject it immediately because it uses a depth-first search. There are many valid topological sort orders of a DAG, but some are nicer for humans than others. A depth-first search tends to move conditionals deep on the page to the top of the order. While we don't currently care because we allow users to organize their conditionals as they see fit, in the future I am thinking that we might enforce a topological sort order on the order of the conditionals in the UI. This would let users see their dependencies automatically.

A second reason I don't care for graph.inc is because it uses input data structures where the edges and vertices are presented in separate arrays. This would mean processing the conditionals to create an intermediate representation before processing. But the time we've done that, and accounted for the extra edges implied by page breaks and field sets, I think it's easier to just calculate it ourselves.

The third reasons is the error generation. We would need a whole separate process just to look for errors, whereas they can easily be detected during the order generation.

- Re webform_get_conditional_sorter() -- that exactly what WebformConditionals::factory() does now. I can leave that feature in so that it might be useful to others. You get class auto-loading as a side benefit.

- I see your point about $node, but I still think the caching and ease-of-use trumps it. None of the data structures are recursive, so print_r won't go crazy.

- No problem on execute, although I think it does just what you'd think -- it executes the conditionals. Suggestion for the name? executeConditionals() seems redundant.

As you can probably see, all this infrastructure is there to support conditionals that change data. I'm not sure how or where this will go, but at a minimum setting values, possibly even with tokens (via ajax).

Just read the Kingdom of Nouns. The difference is that WebformConditionals represents a set of commands. And commands get, well, executed. It is unlike WebformSubmissionLoader::execute(), which the article would argue would better be WebformSubmission::load(). And I agree.

I do see incorporating more conditional logic into WebformConditionals, including the (now redundant) logic of determining which components should be included on a page.

I received a thoughtful off-line comment by torotil that the algorithm in topologicalSort is inadequately documented. I think I agree. Rather than relying on the user to visit the referenced page, I think some comments to explain the algorithm would help future maintainers, and I will add that.

Torotil also wondered whether the algorithm would be easier to understand if the nodes were objects. Indeed there is a open source PHP version which does just that. I considered it, but didn't use it because a) the representation of the nodes and vertices is not convenient for how the existing conditionals are stored b) the objects don't add must flexibility or power and c) creating a bunch of objects takes server time and memory.

He also expressed some concern about performance. I found that with 200 conditionals, each with one source, the algorithm ran on my modest development machine in 3ms, which I think is plenty acceptable. The algorithm is order # conditionals + # source conditions, so it is unlikely to turn horrible with any reasonable webform. Fieldsets and pages contribute implied edges, but even with these, its still linear unless your fieldsets contain sources that are frequently or/and deeply nested.

Re being a controller object, In the ideal world, the conditionals themselves would be the object that would know how to sort themselves, but that's waaaay too big a step to take. It is something that D8 should look at.

I googled and didn't find literature on why controller objects might be undesirable. Any references?

I googled and didn't find literature on why controller objects might be undesirable. Any references?

I'd recommend watching Rich Hickey's Simple Made Easy video. There's nothing inherently wrong with controllers (or any other objects). But the reason for adding this code is because of the (substantially) increased complexity that we're trying to accommodate.

- I see your point about $node, but I still think the caching and ease-of-use trumps it. None of the data structures are recursive, so print_r won't go crazy.

Because PHP is kind of dumb about print_r() (until PHP7), the WebformConditional's protected $node property will cause a recursive loop, because print_r() will print out not only public properties but also all protected and private ones. Since $node contains the WebformConditional class and the WebformConditional class contains its parent nodes, you'll get a recursive error when running print_r() (I haven't check this in the current code, but it's been my previous experience).

I still would feel much, much better if this were put into a separate location rather than in the $node directly. My suggestion about using a function like webform_get_conditional_sorter() would require it's own static cache for each node that needs the sorting functionality. This pattern matches the other Controller-like classes in Drupal, including the controllers classes for saving and loading nodes and other entities. See entity_get_controller.

In PHP 5.3 I get a "error (NULL)" when I try to expand conditionals_sorter using the devel tab. print_r gives '*RECURSION*' for just the node back-link. Both I think are pretty benign side effects of the back-link to the node.

If this recursive $node link was really problematic, we could store the $nid and call node_load($this->nid) in every function. This would rely on the entity cache. Extra work and a performance hit for not much benefit, in my opinion.

Options:

1. Remove factory(), leaving the only constructor call in webform_load_node. Relies on entity caching for intra-request caching and relies on inter-request form caching to prevent re-calculation on multi-page (or previewed) client forms. Least code and the only option that avoids redundant static caching. Models the data best -- data structures which are intimately tied to each other are linked. An object which performs calculations on a specific node is linked to the one specific node that it works on. This is in contrast to entity_get_controller, which returns a new controller of the cached type, for use on any node(s). WebformConditionals isn't really a controller. It is part of webform, just like component['children'] is -- that is, calculated data.
2. Remove factory() and replace with webform_get_conditionals_sorter(). Requires extra caching in that routine. Separates the functionality related to WebformConditionals into two places in the source. Requires adding yet another globally-scoped function to the already-huge webform.module. Avoids the recursive back-link to $node though.
3. Keep factory() and have webform_get_conditionals_sorter() call factory(), relying on the caching in factory(). Similar to 2. Avoids recursive back-link. Better encapsulation of functionality (slightly). Seems needlessly complicated.

This isn't an application of a complicated OOP pattern to a simple problem. The OOP code itself is trivial. The class just serves to keep the code dealing with a specific part of webform's functionality together in a way that is entirely compatible with D7's non-OOP architecture (e.g. all nodes are of type stdClass (ironic, given the acronym STD ;) )).

But, hey, I really just want to get this thing put to bed and move on. It's been a HUGE chunk of work and this is a very minor part of it. If you insist on 2 or 3, I will bow to your wishes. But I think 1 is best for the webform codebase and future webform maintainers. Assuming no other feedback, make the call and I'll commit the code. I want this commit to "cook" in the -dev branch for a while before release as a stable so that it gets extra testing.

In PHP 5.3 I get a "error (NULL)" when I try to expand conditionals_sorter using the devel tab. print_r gives '*RECURSION*' for just the node back-link. Both I think are pretty benign side effects of the back-link to the node.

Ah, I must be thinking of Drupal's debug() function that has this problem. I think Devel's dvm() might also suffer from it, while I know dpm() handles recursion at least.

If you insist on 2 or 3, I will bow to your wishes. But I think 1 is best for the webform codebase and future webform maintainers.

I'd really prefer 2 or 3 here. Removing the factory and adding the webform_get_conditionals_sorter() (or a name that is more fitting if needed) would be my preference (solution 2). I also like this solution because it moves the creation of the WebformConditional object outside of node_load(), meaning it's not going to be instantiated when the form itself isn't even being displayed, as may be the case in administrative listings, Views, certain display modes, edit forms, etc.

Other last feedback:

I'd still like to see "execute" renamed to something more descriptive. Could we make topologicalSort() private or protected? It doesn't seem necessary to have it in the public namespace as it provides no public value.

And I haven't said it already: Good work on taking on this particularly challenging problem. :)

Revised patch per #26.

1. Option 3 implemented:
   $node->webform['conditional_sorter']->method() -> webform_get_conditional_sorter($node)->method().
2. Execute method made more descriptive:
   WebformConditionals::execute-> WebformCondtionals::executeOnSubmission()

+    // Remove any existing data on this and previous pages. If components are hidden, they may
+    // be in the $submission->data but absent entirely from $new_data;
+    $pageMap = webform_get_conditional_sorter($node)->getPageMap();

Even though $pageMap is an object, Drupal's coding standards still indicate this should use underscores instead of camelCase.

+  function topologicalSort() {

This wasn't addressed... should this be protected? I don't think it provides a public value.

+  /**
+   * Executes the conditionals on a submission, removing any data which should
+   * be hidden.
+   */
+  function executeOnSubmission($input_values, $page_num = 0) {

Heh, that wasn't quite what I meant. "Execute" is the indescriptive word here. How about filterInputValues or cleanInputValues?

+function webform_get_conditional_sorter($node) {
+  return WebformConditionals::factory($node);
+}

I see you went with option #3 after all. I think that's a fine approach. Thanks for separating it out of the $node object.

Agree on $pageMap, although I think the standard is wrong. It obfuscates, rather than illuminates, to call a getter method and not be allowed to name the local variable that which was gotten.

Begrudgingly agree on protected for function topologicalSort(). The function is useful for re-sorting, effectively flushing the cache, since that now can't be done by re-instantiating $node->webform['conditional_sorter']. I am not a huge fan of protecting potentially-useful methods whose utility was not envisioned by the module creators. For example, the migrate module has/had tons of this, requiring patches. But in this case, webform maintainers control both the caller and object, so let's make it protected. We can always change it again. I loath the very concept of private. It's saying, "I know better than all who come after me that my way is perfect for all of time."

I think I'm not communicating why I think "execute" is the perfect name. Conditionals are a series of programming commands, not unlike Rules (the module), PHP itself, or machine code. They are executed on a submission in exactly the computer language/hardware sense. A program counter runs down the conditionals, performing them one at a time, reading and writing the submission. Right now, the only two ops are NOP (SHOW) and MAKE NULL (HIDE), but very soon I plan to add others like SET TO XXX. This is the sense in which I mean "execute".

Note that this is completely unlike MyComplicatedVerbifiedNounClass::execute() as described in the amusing tale that you referenced about Java.

Relative to #27, this patch:

1. Changes local variables of $pageMap to $page_map as requested by quicksketch
2. Changes WebformConditionals::topologocalSort to protected as requested by quicksketch. If we need to reset the cache, we'll deal with that in the future.
3. Adds some additional comments to the topologocalSort as requested by torotil.
4. Changes WebformConditals::executeOnSubmission to executeConditionals. The motivation is to make it clear that the actual conditionals are being sorted. If this name remains troublesome, it can be easily addressed in another issue or in a follow-up.

A cache-clear is required to update the class registry.

fenstrat -- this is a big-un. It would be best to get this into D8 sooner rather than later due to the scope of changes.

Issue is no longer "META".

Thanks Dan, everything in #31 sounds good to me!

Right now, the only two ops are NOP (SHOW) and MAKE NULL (HIDE), but very soon I plan to add others like SET TO XXX. This is the sense in which I mean "execute".

Okay that makes a lot more sense. If we were just trimming out invalid values, then "execute" doesn't describe the situation well. If we're going to add other actions besides hiding, then execute will fit in the future. +1

Committed and pushed to 8.x-4.x. Thanks!

Great work here Dan. Sorry I had no time to respond to your architectural questions. The end result looks well done.

One question, for D7, as the class registry needs to be rebuilt is a hook_update_N() needed to trigger one on live sites?

My understanding is that a clear cache is SOP after an update, no?

That said, I think we're in for a bigger hook update if we do the multiple targets / conditional issue. I'm not overly concerned.

SOP on some site perhaps. On large sites a cache clear can be a expensive process. I'm not sure of best practice here after changes to the class registry.

If conditionals are defined out-of-order, then when they are executed on a submission, PHP warning will be generated and the conditionals will not execute correctly on the server.

Committed to 7.x-4.x and 8.x.