Issue summary

When adding a server-side form submission action to an AJAX form that is limited by pages, the action is never triggered because the page url is submitted as /system/ajax (which doesn't match the page limitations).

I'm including a patch that addresses this. The caveat is that because it is a server-side form submission, when used in conjunction with Acquia Lift, the goal is not submitted until the next page request on the site. The alternative is to attach the action as a client-side form submission but this will track all form submissions whether fully submitted or returned for validation errors.

CommentFileSizeAuthor
#8 visitor_actions.ajax_url.8.patch2.11 KBeshta
#3 visitor_actions.ajax_url.3.patch2.13 KBeshta
visitor_actions.ajax_url.0.patch3.14 KBeshta

Comments

katbailey’s picture

katbailey commented 
+++ b/visitor_actions.module
@@ -249,12 +253,16 @@ function visitor_actions_page_build(&$page) {
-function visitor_actions_match_page($pages) {
+function visitor_actions_match_page($pages, $page_context = NULL) {
   // Convert the Drupal path to lowercase
-  $path = drupal_strtolower(drupal_get_path_alias($_GET['q']));
+  $path = empty($page_context['InternalPath']) ? drupal_strtolower(drupal_get_path_alias($_GET['q'])) : $page_context['InternalPath'];

This change feels wrong to me. This function only cares about a path to compare against, so let's give it a path, rather than an array that was assembled for some other purpose entirely.

In fact, I'm not sure it makes to use visitor_actions_get_page_context() at all in this patch, because we may actually be doing away with that eventually. And for this all we need is the current path, right?

eshta’s picture

eshta commented

Well - that was the split - on one hand you may want to have all of the page context to make page matching decisions as logic changes (and the visitor_actions_get_page_context seemed to be separated out into its own logic for a reason even though it is currently only called once).... on the other hand, just passing a single path could make for a simple implementation but any changes to the definition of page context that include a path would be changing in two places. It's true that it (visitor_actions_get_page_context) does extra work that is currently unnecessary for page matching and given that it's bound to change soon I'm not wed to full context approach. I'll simplify.

eshta’s picture

eshta commented
StatusFileSize
new visitor_actions.ajax_url.3.patch2.13 KB

Simplified approach attached.

katbailey’s picture

katbailey commented
Status: Needs review » Reviewed & tested by the community

Yep, looks good to me.

  • Commit cb9c8b1 on 7.x-1.x by eshta: 
    Issue #2283299 by eshta: fix path used for action matching in cases of...
eshta’s picture

eshta commented
Status: Reviewed & tested by the community » Fixed

  • Commit 0878015 on 7.x-1.x by eshta: 
    Issue #2283299 by eshta: Updated storage location for internal path to...
eshta’s picture

eshta commented
StatusFileSize
new visitor_actions.ajax_url.8.patch2.11 KB

The previous patch hit a snag with a naming collision with system_modules_submit. The updated naming within form_state['storage'] corrects this. While the updated code is in the dev branch - I'm attaching an updated patch for completeness and for anyone who finds this via search, etc.

Status: Fixed » Closed (fixed)

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