Make views integration more portable

Unless I'm missing something, I think this ship has sailed for the -4.x branch. I don't see how we can convert to using the form_key as a machine key. I believe that they are (widely) not unique within the webform. I think that their path (including containing fieldsets within a given page).

There would be numerous benefits to using machine keys for webform nodes and for components, including simplified theming and import/export/features. There have also been requests to use UUIDs, which I think is related. I just don't see how we can make a fundamental change without affecting the many existing webforms, webform-related modules, and custom themes. That said, I definitely see a huge benefit. I wish this had been tackled in the 3.x->4.x development. But that effort took a couple of years, even without this change.

Move to the D8 branch??

Yes, the motivation of my patch was to allow a view to run on a similar webform to the original. It prefers the cid, but when the nid is different, it tries to use the form_key.

Kludge is certainly a pejorative description of that functionality. I prefer to think of it as removing some of the brittleness in the original design.

I have sympathy for the need you describe, and I would certainly like to make views less brittle within an installation (different webforms) and when importing webforms or views from another database, such as you describe with features.

The problem is that our standard is not to make non-backward compatible changes to webform within a N.x version. Your patch changes functions which other modules may call. It changes the operation of the views handlers, creating incompatibilities with any developer who has subclassed them. It relies on the title of a webform as being unique, which it isn't guaranteed to be. It relies on form_keys to be unique, which they aren't.

In short, this is a great idea, and I would have been happy to include it (after testing) in webform 4.0. I don't anticipate a webform 7.x-5.0, so I'm suggesting we move this to the 8.x queue.

Whether desirable or not, Drupal doesn't "cling to backwards compatibility" between major version (e.g. 7.x -> 8.x). The core maintainers clearly freely make changes, even when they introduce incompatibility with relatively little reward (such as renaming functions or changing parameter orders).

Within a given Drupal version, 7.x in this case, we as module maintainer have a duty to maintain backward compatibility with major version numbers -- 7.x-4.x in this case. This means both functional and API / programming compatibility. Site maintainers need to be assured that they can install minor point updates without fear of their site breaking. This has been quicksketch's standard for the history of webform, and I believe is widely held standard in other contrib modules.

I do agree that the change you suggest is relatively minor, but it is not without risks to the updater. form_key's are very frequently not unique because people use the same form_key within different fieldsets (e.g. name, street, city, state, phone -- both home and work) and between different pages. I don't think this is a good design, and I would like to change it too. But we simply cannot within the 7.x-4.x branch.

Your patch changes webform_component_list. I believe this list makes its way into the node as $node->webform['components']. This is the sort of general purpose routine that another module author (possibly a custom, non-contrib module) might use. I do not think it is safe to change what it returns. I myself rely on this in my custom module.

Unless you can accomplish what you want in a fully backward compatible manner, I don't see this moving forward. "Fully" means that no site that works properly with the current 7.x-4.x branch would break after the patch. I also would not want to use node title as a key. Using the UUID module, if installed, might be a work-around.

We are on the same side on the technical merits. But pragmatically, with lots of sites still using 7.x-3.x -- and lots of webform-related modules stuck in -3.x land, I don't see 7.x-5.x happening anytime soon.

I hope I didn't mislead you, as the change to webform_component_list is but one of the incompatibilities, right?

This issue proposes to solve an issue I've run into on a couple of projects.

On my current project, I'm capturing all configuration with Features, including a Webform submissions view. The site will host a number of registration forms. Each will be different, but they will all have a set of common components: first_name, last_name, email, etc. Because the site is deployed with Features, there is no source node; thus, I need to be able to key off of form_key and not a combination of nid/cid.

It sounds like @solotandem is going to post a reworked patch. I'll wait to do any testing until that patch has been posted.

@solotandem, thanks for tackling this issue. @DanChadwick, thanks for responding to this issue so quickly. Too often we never hear back from a module's maintainer.

@solotandem -- thank you for your great work. I've glanced over the patch and I will find some time soon to apply it and look at it in detail and prepare some detailed comments. I'm very encouraged!

Ironically, I was thinking about this issue earlier when I was out walking and it occurred to me that we could use both the nid and a webform machine name, which seems to be the same solution you arrived at.

The webform machine name could also be used for theming. "webform_client_form_survey_alter()" is much nicer than "webform_client_form_123_alter()", right?

The same concept could be applied to the cid, creating a guaranteed-unique-within-the-webform component machine name, leaving the form_key for theming only. I'm not sure about this....

We also need to be mindful of scale. We need to handle (and update) sites with 10,000 to 100,000 webforms, with 100's of components per webform, with 1,000,000 components, with 1,000,000 to 10,000,000 submissions, etc.

And, of course, we need to be mindful of existing views that cannot be updated. Many of these will be in code. I didn't see anything in your patch to the contrary.

I would love to see others involved in the review of this feature and a patch of this magnitude. Again, thank you for taking this on.

Again, thank you for your significant effort on this issue. Comments are in the order I saw/thought of them, not in any sort of priority order.

General

1. If we make the default webform_name be the NID rather than webform_NID, then if we use the webform_name for theming, all the existing theme override will nicely keep working.
2. I don't want to put the machine name in the forefront of the UI. This is an esoteric feature of use to perhaps 1% of the webform installed base. I would rather see the UI moved to the advanced fieldset in the webform form settings. I'd like to see the default continue to be the nid, rather than trying to make a unique name out of the node title. We need to remember that there are sites with 100,000 webform nodes. There are no sites that have 100,000 field API fields. There will be lots of name collisions in some databases.
3. I'm not sure about if/when the webform_name should be immutable. If we allow it to be changed, it may make deployment issues easier. OTOH, we risk allowing the user to break their views (and possibly theme function). Hmmmm.
4. We also need to think carefully about how views reference their webform node(s). For example, if we store the webform_name and it no longer matches any webform, they we could try it again as a NID if it's numeric. Or if we lock the machine name, then this isn't an issue anymore, but we may be locking users into a machine name they don't like. (How many times have you had to delete a field and re-create it because you changed your mind about the machine name? For me, the answer is lots, and I hated it every time.)
5. Similarly how do views refer to components? The cid is guaranteed to give the right component for the webform that was used to define the view. The form_key might give the right component when the view was imported by features or when querying on a set of related webforms (my use case). Maybe the answer is to let the user (view creator) decide?

webform.install

1. Let's take a queue from the field api can call the 'name' field 'webform_name'. This may help avoid confusion when tables are joined.
2. Since you will be querying the database looking for components by name, the field needs an index. The index should be unique.
3. update_7422 can't refer to webform_schema() because in the future, the schema may have changed. See the documentation on hook_update for details.
4. update_7422 contains some odd code, setting variables where constants would do just fine.
5. update_7422 contains code to not update rows that already have a value. Presumably this is to protect against partial updates. This initial query will require a length table scan since the index can't be used yet. I think wrapping this in a transaction would be better. We need to do some performance measurements on large databases to see if a batch operations is needed here. Probably not.
6. update_7422 should create the unique index after the data is in the column.

webform.component.inc

1. webform_component_load has a very awkward signature and needed two helpers. We can make it much cleaner and avoid the helpers by using the query spec array similar to the deprecated node_load parameter. For example webform_component_load(array('form_key' => $keys_array, 'webform_name' => $webform_name)); Then within the function, you can just loop though the argument creating condition clauses. For example, $query->condition($key, $value, is_array($value) ? 'IN' : '=');
2. webform_component_list also has a bit of an awkward signature. I'd suggest adding $key = 'cid' rather than $cid_key = TRUE to the signature. The code in the routine is easier, and the call is more self-documenting.

webform.module

1. Comments related to default webform_name and location in the UI.
2. Validation for a webform name should be both unique and non-numeric (reserving all numeric names for automatically generated names)
3. _webform_name_exists -- or its equivalent after incorporating these comments -- cannot retrieve all the names into a PHP array -- way too big. You need to just query on the desired name, and also check for non-numeric.

webform_hander_relationship_submission_data.inc

1. As I recall (without looking closely), the use case for this handler is to establish a relationship to only one webform so that the fields can be added to the view knowing the webform components. I'm unsure how this issue changes that.

I didn't review the handlers closely because I think there's plenty to discuss and address. Phew. Time for coffee.

Work on D8 is accelerating. It's starting to be do-or-die for this issue in D7. Just sayin'. :)

I think this issue totally has merit, but I would be in over my head if I tried to jump in. I created another issue that is narrower in scope that seeks to solve the issue of keying off the form_key. I'm not trying to derail this issue, but I needed to get something to production for a client, and now I'm sharing with the community for consideration.

#2506429: Provide Option to Key on 'form_key' in Webform Submission Views

Closing for lack of activity. Please do re-open if there is further progress on this patch.