I ran into a nasty bug that occurs when a webform is on a panelized node page. (For example, use Panelizer to panelize the Webform content type, then put the webform as one of the panel panes.)

In this scenario, if the webform is a multistep form, then clicking the "previous" button on page 2 won't return to the previous page. Furthermore, it will throw away all the form data that you entered on that previous page.

The bug ultimately comes from the fact that in this configuration, the same webform gets built twice on the same page request. The second time it is built overwrites the good data from the first time it is built.

CommentFileSizeAuthor
#5 webform_panels_safe-2215947.patch1.04 KBquicksketch
#1 webform-double-render-panelizer-2215947-1.patch1.13 KBDavid_Rothstein

Comments

David_Rothstein’s picture

David_Rothstein commented
Status: Active » Needs review
StatusFileSize
new webform-double-render-panelizer-2215947-1.patch1.13 KB

Here is a patch. Probably not the right long-term fix, but I needed something that worked and so far this seems to do the job.

quicksketch’s picture

quicksketch
he/him
commented

The bug ultimately comes from the fact that in this configuration, the same webform gets built twice on the same page request. The second time it is built overwrites the good data from the first time it is built.

This has come up quite a few times for users of Panels and Webform together. Do you know the reason for why the Webform gets called twice per page? Would it be possible to fix this in Panels to prevent it from trying to display node content twice per page?

David_Rothstein’s picture

David_Rothstein commented

I did a little research; it turns out there's more than one way for it to happen.

On the site I originally saw this on, it was actually due to this Ctools patch: #1760384-13: Update node_view.inc to execute the normal Drupal hooks. The first node_view() came from the code added there in Page Manager (in the node view menu callback), and the second came from the pane that actually contained the webform.

Without that patch, looks like you need to put two node-related panes on the page to trigger the bug, but of course that's not uncommon. I was able to do it, for example, by adding the "This node: Node links" and "This node: Webform" panes to the page (in that order). The code within each pane results in node_build_content() being called, so hook_node_view() is invoked twice.

I am not sure Panels/Ctools could easily avoid this because the individual panes can do what they want, and some need to render the node (furthermore, one pane might need to render the node using a different view mode than another pane, in which case two separate calls are definitely necessary).

quicksketch’s picture

quicksketch
he/him
commented

Okay, seems good to me. Let's make a few adjustments to the patch though:

- We no longer need the if/else statement for !empty($node->webform['components']). There's a check at the top of webform_node_view() that bails out early if there aren't any components.
- Let's rewrite the docs a bit to be more generic but give Panels as an example:

+    // Avoid building the same form twice on the same page request. This can happen
+    // if node_view() is called multiple times for the same node, such as when a webform
+    // node is being built with Panels.

...eh, I'm not sure it's better or worse.

- I can't see why the cache key needs to include a user ID: $cache_id = $node->nid . ':' . $user->uid;. Can the same form be built for 2 user IDs in a single request?

quicksketch’s picture

quicksketch
he/him
commented
Status: Needs review » Fixed
StatusFileSize
new webform_panels_safe-2215947.patch1.04 KB

I've committed this simplified patch that removes the unnecessary if/else statement. Thanks @David_Rothstein!

  • Commit ebfe74a on 7.x-4.x by quicksketch: 
    Issue #2215947 by David_Rothstein: "Previous" button on multistep forms...
fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented
Version: 7.x-4.x-dev » 8.x-4.x-dev
Assigned: Unassigned » fenstrat
Status: Fixed » Patch (to be ported)

Needs porting to 8.x-4.x.

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented
Version: 8.x-4.x-dev » 7.x-4.x-dev
Assigned: fenstrat » Unassigned
Status: Patch (to be ported) » Fixed

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

  • Commit c4694aa on 8.x-4.x by fenstrat: 
    Issue #2215947 by quicksketch, David_Rothstein: "Previous" button on...

Status: Fixed » Closed (fixed)

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

torotil’s picture

torotil commented
Version: 7.x-4.x-dev » 7.x-3.x-dev
Status: Closed (fixed) » Active
Related issues: +#2273425: multi page webforms not working in webform as block

This is still an issue in 7.x-3.x. Moreover the very same patch also applies to 7.x-3.x. Could someone please cherry-pick that commit into 7.x-3.x?

torotil’s picture

torotil commented
Related issues: +#766146: Support multiple forms with same $form_id on the same page

… also linking the related (and yet unfixed) core-bug for documentation purposes.

danchadwick’s picture

danchadwick commented

@torotil - The maintainers have put a lot of effort into releasing the 7.x-4.x branch. I would suggest you upgrade and/or put the effort into upgrading any webform-related modules that you need.

torotil’s picture

torotil commented

Hi, I had hoped that there would be at least some transition period where 7.x-3.x still gets critical bug-fixes like this, and the release of 7.x-4.x.

Webform 7.x-4.x is out since 20days. The bug is known since half a year and has been fixed for 5 months.

I think it's not too much to ask that major bugs are fixed for a release that still 100.000s of sites are using. All it needs is:

git checkout 7.x-3.x
git cherry-pick c4694aa
git push

… and from time to time release some bugfix release.

I'm maintainer of several webform based modules. Instead of getting them ported to webform-7.x-4.x I'm spending my time pleading for major bugs to be fixed in a stable and still supported release and managing dozens of patches in the meanwhile.

Thanks for your support.

danchadwick’s picture

danchadwick commented

@torotil -- Maybe approach quicksketch about being a maintainer for the -3.x branch(es)? Might solve all the problems? I am devoting the (limited) resources I have to pushing the 4.x branch forward (and keeping the 8.x branch in sync). That leaves essentially nothing for the 6.x-3.x and 7.x-3.x branches. This would certainly have my support.

danchadwick’s picture

danchadwick commented
Version: 7.x-3.x-dev » 7.x-4.x-dev
Priority: Major » Normal
Status: Active » Closed (fixed)

Closing again for lack of maintenance on 7.x-3.x. The bug is fixed in the branches that are actively maintained. If someone wants to take a lead in supporting 7.x-3.x, please approach quicksketch.

torotil’s picture

torotil commented

I've cherry-picked this into 7.x-3.x and it will be part of the 7.x-3.23 release.

torotil’s picture

torotil commented

I've cherry-picked this into 7.x-3.x and it will be part of the 7.x-3.23 release.