Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tauno’s picture

Status: Active » Needs work
FileSize
2.06 KB

I was curious about how difficult this would be so I rolled an ugly 2-minute patch. It works fine, but probably isn't the cleanest approach. The patch uses the variable value for search_api_solr_connection_class as the class to use and defaults to the current class.

populist’s picture

Here is an updated patch based on the work tauno did that overrides the solr connection class. My patch uses a variable override (similar to the apache solr module) which I think is a little more generalized.

populist’s picture

And a quick reroll to do the operations in reverse.

tauno’s picture

Status: Needs work » Needs review

#3 looks good and works in my basic test forcing a the solr server hostname.

@populist, thanks for jumping in. Now that I look at it, I'm pretty sure my patch was broken. At least it showed there weren't many changes necessary. Looking forward to search_api w/ solr hopefully being available on pantheon.

joshk’s picture

I think we can actually do this even cleaner by overriding the Transport class. Took me a while to figure out as there's a fair amount of code in the class here that's been factored out (isn't used, but still in the file).

stopshinal’s picture

FileSize
3.2 KB

Here is a rerolled patch to head, I cant confirm this works. Figure i'll toss this out here anyways.

stopshinal’s picture

Ok, well I had the chance to try this out, didn't work

stopshinal’s picture

I'm getting this error when I try to apply: fatal: corrupt patch at line 62

populist’s picture

I was able to get the patch in #6 to apply cleanly to the latest dev release.

arrubiu’s picture

I tried patch #6 with latest stable of the module but I can't connect to the solr server.
Also with -dev version

arrubiu’s picture

Now it's working.
I've applied patch #3 with "patch filename.patch"

joshk’s picture

Looks like there's a new RC2 out, which doens't include this patch and also doesn't work on Pantheon.

Any chance this flexibility can be more elegantly integrated?

drunken monkey’s picture

Status: Needs review » Needs work

Seems to work fine and I'm also fine with the approach. Can't really remember, but there seemed to be some reason against it, or I would've done it this way in the first place. Using a hardcoded class name seemed awefully bad practice.

Anyways, there are still a few issues that need to be addressed:
- The patch adds whitespace and formatting errors.
- The variable should be called search_api_solr_connection_class, it's not the service class we're replacing.
- Unless I'm mistaken, $class::method() won't work in PHP 5.2 (but can't test and didn't find the docs right now). Please either test and confirm this works in PHP 5.2, or use a safer way to call static methods.
- Note the variable's existence in README.txt (add an extra "Hidden configuration" header above "Developers").
- Delete the variable in search_api_solr_uninstall().

joshk’s picture

I believe you're correct that the patch does in fact work against RC2. I'll take a fresh look this week. Thanks for the comments!

populist’s picture

Here is an updated patch that applies cleanly against RC2. It addresses the following considerations raised in #13:

-- changes name of variable to search_api_solr_connection_class
-- adds documentation and example code to README.txt
-- deletes the variable in the search_api_solr_uninstall()

It does not address the following issues:

-- refactoring the $class::method() to work cleanly with PHP 5.2.

joshk’s picture

I can roll another version that uses call_user_func() to implement the class call. Done that workaround before. I'll post a new patch sometime later today or tomorrow.

erics14’s picture

Joshk, when do you expect the revised patch to be available?

dellad’s picture

subscribed

drewish’s picture

dellad, you don't have to comment to subscribe any more. Just click the big Follow button at the top right of the page.

nafmarcus’s picture

Can I second #17?

I'm all ready to deploy Solr on my pantheon site using the search api module and would love to use a clean, Pantheon patch. Otherwise am I to assume I should use patch from #15?

nafmarcus’s picture

I installed the patch on my local machine, restarted my Solr server and if clear cache or to to /admin I am now receiving the following error:

Parse error: syntax error, unexpected T_PAAMAYIM_NEKUDOTAYIM in /Users/nafmarcus/GITB/itcs1/sites/all/modules/contrib/search_api_solr/service.inc on line 850

Do I need to configure some file by hand?

erics14’s picture

Roll on the day when this is solved in RC3!

frob’s picture

Not exactly sure why, but this patch doesn't apply at all.

git apply --stat 1407282-search-api-custom-connection-class.patch 
 0 files changed, 0 insertions(+), 0 deletions(-)
joshk’s picture

Updated patch using call_user_func() to safely invoke a variable class name as per example #4 in the PHP docs. Also cleaned up some whitespace.

randallknutson’s picture

Status: Needs work » Needs review
FileSize
4.47 KB

I found a couple issues trying to use the patch in #24.

1. The latest version of pantheon_apachesolr (at least the latest that I'm aware of) actually sets the variable "search_api_solr_service_class" not "search_api_solr_connection_class" which took me a while to figure out why it wasn't working. (I'm sure Josh knows more about the variable name than I do but this is what is currently working on a fully patched pantheon site.)
2. There is a type around line 853 of service.inc which has the parenthesis in the wrong place

        $key = call_user_func(array($class, 'phrase', $key));

should be

        $key = call_user_func(array($class, 'phrase'), $key);

I've attached a patch with these two changes and have tested it on pantheon and it works with the latest 7.14 from them with pantheon_apachesolr.

joshk’s picture

Thanks for that, Randall! Must have been an oversight on my end. :P

drunken monkey’s picture

1. The latest version of pantheon_apachesolr (at least the latest that I'm aware of) actually sets the variable "search_api_solr_service_class" not "search_api_solr_connection_class" which took me a while to figure out why it wasn't working. (I'm sure Josh knows more about the variable name than I do but this is what is currently working on a fully patched pantheon site.)

See my comment in #13:

- The variable should be called search_api_solr_connection_class, it's not the service class we're replacing.

Also, the lines in README.txt shouldn't be longer than 80 characters, there were some whitespace issues and the text in the README should better explain what is replaced, instead of citing an example.

Please see if the attached patch is OK for you!

populist’s picture

I checked this out on Pantheon and this looks good to me!

It requires a change, of course, to our Pantheon Apache Solr connector module, but I created a branch for that with this commit (https://github.com/pantheon-systems/drops-7/commit/a936b4b7f86608f34d679...) to help folks on their way.

arrubiu’s picture

So to apply #27 I've to wait for the patch to pantheon apache solr?

joshk’s picture

The patch at #27 should work with Pantheon now.

arrubiu’s picture

With the last stable version of search_api_solr?

gmclelland’s picture

@sergio.durzu - The patch applies fine against 7.x-1.0-rc2

desmondmorris’s picture

If you disable Pantheon Solr Search locally (to test with a local solr instance), you will get missing class errors for "PantheonSearchApiSolrService". I updated the patch from #27 and added a quick check to determine whether the class name stored in the search_api_solr_connection_class variable exists or not.

mkalbere’s picture

Would this allow to use SolrPhpClient/apache/Solr/HttpTRansport/Curl.php , or it has nothing to do with this ?
By default search_api_solr use file_get_content to communicate with Solr. So it opens a LOTS of tcp connection. Using curl would allow to use "keep alive" connection. Is there a way to do that ?

pirog’s picture

Here is a rerolled patch for the latest dev (RC2+13) with a couple of small tweaks because it looks like some of the code has changed significantly since #33.

pirog’s picture

actually... im getting weird results with #35... so i think we need to do some more here

populist’s picture

Here is an updated version of the patch in #33 which addresses the latest release of RC3.

pirog’s picture

tested this out on my local solr server, after updating the schema, and it seems to work fine

gmclelland’s picture

Just tried the patch in #37 with 1.0-rc3.

With the Pantheon modules enabled, when clearing an index Locally on my computer I get this error on /admin/config/search/search_api/index/node_index/status:

Strict warning: Creating default object from empty value in PanteheonSearchApiSolrHttpTransport->performHttpRequest() (line 153 of /Users/glenn/websites/7c9d097f-0bc1-457d-8314-1d9e37f102bf/modules/pantheon/pantheon_apachesolr/Pantheon_Search_Api_Solr_Service.php).

I couldn't index my nodes until I disabled the Pantheon modules locally.

After the nodes were reindexed I ran a search with a search_api_solr view I got the following error that doesn't want to go away:
Warning: call_user_func() expects parameter 1 to be a valid callback, class 'PantheonSearchApiSolrService' not found in SearchApiSolrService->flattenKeys() (line 892 of /Users/glenn/websites/7c9d097f-0bc1-457d-8314-1d9e37f102bf/profiles/cmf/modules/contrib/search_api_solr/service.inc). (x2)

Let me know if you need any more info. I did clear the caches.

Note: the searches made with views and search_api_solr and search_api_autocomplete still function correctly.

joshk’s picture

Still working on support on the platform for rc3. The transport method and schema both changed a lot. We hope to have something in the next couple days.

pirog’s picture

yes. we were having the same problems with kalabox.

@joshk... would be interested in seeing what you guys end up doing so we can try to preserve kalabox/pantheon parity.

tim.plunkett’s picture

Note, if you're trying to get this working on Pantheon right now, 7.x-1.0-rc3 does not work.

I'm not sure which commit broke it, but rolling all the way back to 7.x-1.0-rc2 works great with this patch (a reroll of #33).

ufku’s picture

Here is the patch for RC3.

Additionally, PantheonSearchApiSolrService has some bug with POST method which is the default in RC3.
A workaround is to select "GET" as the "HTTP method" under solr server settings.

BarisW’s picture

ufku; thanks for adding this patch. I will test it ASAP.

I'm wondering if you've tested autocompletion functionality, as the RC2 version doesn't seem to work.

gmclelland’s picture

Anybody have success yet with RC3 on Pantheon and in a local install yet?

@joshk - any updates?

BarisW’s picture

Status: Needs review » Needs work

The patch doesn't seem to work. I cannot index anymore (localhost).

Warning: call_user_func() expects parameter 1 to be a valid callback, class 'PantheonSearchApiSolrService' not found in SearchApiSolrService->createFilterQuery() (line 958 of /sites/all/modules/patched/search_api_solr/service.inc).
Warning: call_user_func() expects parameter 1 to be a valid callback, class 'PantheonSearchApiSolrService' not found in SearchApiSolrService->formatFilterValue() (line 998 of /sites/all/modules/patched/search_api_solr/service.inc).

I've tried setting it to GET, no luck either.

BarisW’s picture

Status: Needs work » Needs review

Hmm, it seemed to be a variable still set to the old class.

Running drush vset search_api_solr_connection_class SearchApiSolrConnection fixed it.

Local all seems to work fine. Will need to test it on Pantheon as well, so setting it back to Needs review.

BarisW’s picture

Status: Needs review » Reviewed & tested by the community

Also tested on Pantheon, and works fine ;)

THe only missing on the patch is a hook_update_N() to set (or maybe delete?) the search_api_solr_connection_class variable.
However, on Pantheon, this variable should be set to PantheonSearchApiSolrService, so in this case Pantheon could add it to their settings.php?

BarisW’s picture

Status: Reviewed & tested by the community » Needs review

Hmm, I think I was over-excited here. I thought all worked fine, but recently it stopped working (and I cannot point the cause of this). I've tried updating Search API to 1.4, but no luck. I believe this needs more and more proper testing.

Apologies for the miscommunication.

omega8cc’s picture

For reference, please check the other issue I just closed as a duplicate - #1844250: Broken Solr 1.4 compatibility results with error: The Solr server could not be reached. - More details:

Commerce Kickstart 2.5 *without* any patch - http://drupal.org/node/1844250#comment-7241138

Panopoly head with patch from #37 above - http://drupal.org/node/1844250#comment-7241192

drunken monkey’s picture

Status: Needs review » Needs work

#1846254: Remove the SolrPhpClient dependency has introduced some relevant changes, so this would need a re-roll. I think it'd be best to set the connection class to the variable at service class creation time (i.e., in __construct()), then it will automatically be used everywhere.
We also introduced an interface for connection classes, which the README.txt regarding this feature should probably mention.

populist’s picture

Status: Needs work » Needs review
FileSize
2.9 KB

Awesome to see the SolrPHPClient dependency removed. This simplified a *lot* of the work needed to automatically configure Solr on a platform.

The attached patch provides support for overriding the SearchApiSolrService class as part of the search_api_solr_search_api_service_info(). This is a hidden variable as documented by the README. The patch also provides hidden configuration for passing a custom stream wrapper context to drupal_http_request() which is entirely optional and is documented in the hidden configuration section fo the README.

drunken monkey’s picture

Wouldn't this all also be possible with a simple hook_search_api_service_info_alter() implementation? I don't think the benefit warrants the additional complexity (even though its minimal).

populist’s picture

Component: Code » Documentation
FileSize
862 bytes

Thanks for mentioning this hook and that would indeed allow us to simplify this patch even further. In fact, I just turned this issue into a documentation task and have an attached patch to document the method suggested in #53 to change the connection class.

I also opened a related, but necessary issue, in #1990422: Provide Programatic Support for Stream Wrapper Contexts for Custom Connection Classes to provide programatic support for adding Stream Wrapper Contexts. This will allow folks to override the connection class using the documentation here and then pass along Stream Wrapper Contexts to makeHttpRequest in Search API Solr.

drunken monkey’s picture

Note that there's also a setConnectionClass() method on the service class, which allows you to do this without overriding the service class.
Also, since overriding the service class is possible for all services with that hook, not just for Solr, I'm not sure we should document this separately here. Maybe just add a child page to the Search API documentation.

Tim Corkerton’s picture

i'm trying to use Search API with Pantheon and there's a note here http://helpdesk.getpantheon.com/customer/portal/articles/361249-apache-s... which basically says you need to use rc2 + a patch to use Search API on Pantheon. This was written before rc4 was released. Is is possible to get rc4 running with Pantheon? Do I need to apply the patch discussed above. Thanks

seanr’s picture

I tried RC4 unsuccessfully. We really need to get a fix in for this - it's blocking a planned release (though we're still about a month away). Using an old and potentially buggy version with a patch is not an ideal solution.

Anonymous’s picture

+1, same issue here. Still running patched RC2.

drunken monkey’s picture

Since I haven't used or examined Pantheon, I don't really know what would have to happen for users to be able to use RC4 or later with Pantheon. It seems they use a variable to set the connection class, which we still don't allow by default – but perhaps, with this newer version, this wouldn't be needed anymore and they could do the same with an alter hook.
In any case, you'd have to contact the Pantheon support about that, I can't tell you for sure.

scor’s picture

In case it helps, the connection class used by Pantheon is on githuh: https://github.com/pantheon-systems/drops-7/blob/master/modules/pantheon...

drunken monkey’s picture

Status: Needs review » Postponed (maintainer needs more info)

Until we hear from some Pantheon developer, I don't think we can do anything here. I agree with all the last posters, this should really be changed. RC 2 is now over a year old and there have been huge changes since then.

Anonymous’s picture

I am a Pantheon user. Pantheon developers released an updated Pantheon module 3 weeks ago that support the Search API Solr RC5, no patch required. I haven't done any extensive testing yet, but it seems to work properly.

drunken monkey’s picture

Status: Postponed (maintainer needs more info) » Fixed

Oh, excellent news, thanks for reporting this here!
Seems this issue is resolved, then. Or should we still change something?

colan’s picture

It would be great if a Pantheon user with a support plan could get in touch with them and tell them to update this page: http://helpdesk.getpantheon.com/customer/portal/articles/361249-enabling...

Until that happens, the page will keep referring folks over here, causing confusion.

Once that's updated, we can close this.

Status: Fixed » Closed (fixed)

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