Context

I'm trying to add übercart's 'add-to-cart' button to Apachesolr results. I'm overriding the search templates in my template.php and do something like this:

<theme>_preprocess_search_result(&$variables) {
  $result = $variables['result'];
  $product = node_load($result['node']->nid);
 
  $variables['addtocart'] = theme('uc_product_add_to_cart', $product);
}

Not the prettiest code, but it does the job: theme_uc_product_add_to_cart is a wrapper for drupal_get_form which fetches the rendered 'add-to-cart' form and passes it on to the template.

Problem

The form doesn't work. If you click 'add to cart', you are redirected back to the results page but without any results showing. The product you wanted to add wasn't added to the cart.

Cause

A conflict between the Apachesolr and Drupal Search modules and the form API which prevents the form from being processed. I refer to issue #497206: Avoid search conflicts with other forms, use menu API instead of search_get_keys() for a solution and a better explanation. The issue also contains a patch against Drupal core's search_view() function

Solution

Apachesolr overrides search_view() with it's own apachesolr_search_view() which is basically the same code. I implemented the same change submitted in #497206: Avoid search conflicts with other forms, use menu API instead of search_get_keys(). That fixed the issue for me.

CommentFileSizeAuthor
#23 614644-ignore-whitespace.patch1.63 KBjpmckinney
#23 614644-23.patch6.27 KBjpmckinney
#20 614644-remove-post-check-6x-1x-20.patch3.42 KBpwolanin
#14 614644-remove-post-check-6x-1x-14.patch3.41 KBpwolanin
#13 614644-remove-post-check-6x-1x-13.patch3.38 KBpwolanin
#1 forms_in_search-614644.patch840 bytesnetsensei

Comments

netsensei’s picture

netsensei commented
StatusFileSize
new forms_in_search-614644.patch840 bytes

Patch attached.

twiik’s picture

twiik commented

Patch worked perfectly for me.

Thanks.

robertdouglass’s picture

robertdouglass commented
Status: Active » Needs review

Patches get reviewed quicker if set to "needs review" =)

ivrh’s picture

ivrh commented

Confirming the same issue with standard drupal search (drupal 6.15)

robertdouglass’s picture

robertdouglass commented
Version: 6.x-2.x-dev » 6.x-1.x-dev
Status: Needs review » Patch (to be ported)

Great - thanks!

Committed to Drupal 6.x-2.x

robertdouglass’s picture

robertdouglass commented
Status: Patch (to be ported) » Fixed

#614644 by netsensei | robertDouglass: Fixed Forms attached to Apachesolr search results won't work.

Committed to 6.1 as well.

pwolanin’s picture

pwolanin commented

argh - this was dup with another issue with possibly more targeted fix. #735318: Remove unnecessary POST check

netsensei’s picture

netsensei commented

I agree with pwolanin. Checking $_POST is not the best approach in the first place. The root of the problem can be traced to this issue:

#600424: search.module pulls arguments directly from $_GET rather than using the menu system

In a nutshell: after a submit, the original Search module would redirect the user to the _view page and passes the posted search data to the callback as arguments in the path. As meba points out in #735318: Remove unnecessary POST check

Problem: outside the FAPI context (a form builder function which passes you $form_state through drupal_get_form, you don't really have any good means to check if there was any form submission or discern (in case of multiple forms as described in this issue) which form was actually submitted.

This is a Drupal issue, not a ApacheSolr issue.

robertdouglass’s picture

robertdouglass commented
Status: Fixed » Needs work

So where are we? Do we need a rollback? A refinement? I'm marking as needs work until we're sure we've got a good solution.

robertdouglass’s picture

robertdouglass commented
Version: 6.x-1.x-dev » 6.x-2.x-dev
netsensei’s picture

netsensei commented

I've rephrased and went a bit deeper on the issue here:

#497206: Avoid search conflicts with other forms, use menu API instead of search_get_keys()

From my point of view: Drupal's FAPI/Page callback systems just don't cover this kind of edge cases.
It probably deserves it's own issue which points to all related issues.

It's not an easy issue to tackle. Not the kind you can fix with a small patch as it affects multiple major subsystems. Last night, I jotted down some notes and the longer I kept pondering about it, the deeper the rabbit hole seemed to go.

Personally, I don't think that, within the current boundaries, you can work around it without checking $_POST in your page callback. What my patch really does is no attempt to remove $_POST, but fix the current context to make other forms work when called in the 'search results' page. You could wait for a more definitive fix which also removes $_POST, but I think that will take time to implement in Drupal core while people who want to use forms in search results, are still pretty much cornered.

Since it's such a small change (not even a regression), I guess it wouldn't hurt to add it as does just that: patch up an 'awfully bad' situation to 'just a bad' situation.

Anyway, I've contacted pwolanin by mail. He agreed to get together with me at DrupalConSF to take a look at this. If you're interested in pitching in: contact me.

robertdouglass’s picture

robertdouglass commented

Yes - make sure we meet at DCSF.

pwolanin’s picture

pwolanin commented
Version: 6.x-2.x-dev » 6.x-1.x-dev
Status: Needs work » Needs review
StatusFileSize
new 614644-remove-post-check-6x-1x-13.patch3.38 KB

Looking at that core issues looks like Jennifer has come up with a really nice solution.

pwolanin’s picture

pwolanin commented
StatusFileSize
new 614644-remove-post-check-6x-1x-14.patch3.41 KB

slightly better code comment.

dpalmer’s picture

dpalmer commented

Does the above patch work for 6.2.x?

EDIT:

Apparently not, I tried applying the patch and received these errors:

patching file apachesolr_search.module
Hunk #1 FAILED at 141.
1 out of 1 hunk FAILED -- saving rejects to file apachesolr_search.module.rej
pwolanin’s picture

pwolanin commented

shoudl be an easy port - can you try it with 6.x-1.x? I'd like to get this fixed before 6.x-1.1

dpalmer’s picture

dpalmer commented

@pwolanin

I set up a sandbox site just with apachesolr 6.1.x and I tried to apply the patch but it failed. See my CLI output here:

apachesolr:dgpalmer $ pwd
/Applications/MAMP/htdocs/sandbox/sites/all/modules/contrib/apachesolr
apachesolr:dgpalmer $ cvs status apachesolr_search.module
===================================================================
File: apachesolr_search.module  Status: Up-to-date

   Working revision:    1.1.2.6.2.147 
   Repository revision: 1.1.2.6.2.147   /cvs/drupal-contrib/contributions/modules/apachesolr/apachesolr_search.module,v
   Commit Identifier:   JbLbrctpLufqUYBu
   Sticky Tag:          DRUPAL-6--1 (branch: 1.1.2.6.2)
   Sticky Date:         (none)
   Sticky Options:      (none)

apachesolr:dgpalmer $ patch -p0 < 614644-remove-post-check-6x-1x-14.patch  
patching file apachesolr_search.module
Hunk #1 FAILED at 141.
1 out of 1 hunk FAILED -- saving rejects to file apachesolr_search.module.rej

I just checked out the 6.1.x from CVS so I'm surprised that this patch worked for other people...

pwolanin’s picture

pwolanin commented

Let me try - possible I rolled it against the last release versus the dev code.

pwolanin’s picture

pwolanin commented

Ah, indeed - Robert made a change here in #6 as a partial fix for this problem http://drupalcode.org/viewvc/drupal/contributions/modules/apachesolr/apa... because of that the patch against 6.x-1.0 didn't apply.

pwolanin’s picture

pwolanin commented
StatusFileSize
new 614644-remove-post-check-6x-1x-20.patch3.42 KB

This should apply.

pwolanin’s picture

pwolanin commented
Version: 6.x-1.x-dev » 6.x-2.x-dev
Status: Needs review » Patch (to be ported)

sadly, this doesn't apply cleanly to 6.x-2.x either, but committing to 6.x-1.x.

dpalmer’s picture

dpalmer commented

@pwowalin I was able to get the 6.x-1.x patch to work fine, but as you mentioned it doens't work for the 6.x-2.x. I tried looking at the 6.x.2.x code to see if I could manually fit the patch in somewhere, but unfortunately the code & functions are quite different in the 2 different versions. Any idea on an ETA on when you could release a 6.x-2.x patch?

jpmckinney’s picture

jpmckinney commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new 614644-23.patch6.27 KB
new 614644-ignore-whitespace.patch1.63 KB

Included regression fix from #853564: Checkbox to do new search with existing filters is missing. Attaching patch with -w flag so you can see that the changes aren't that significant.

jpmckinney’s picture

jpmckinney commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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