Using POST for all select queries

Just switch the method to "GET" in the options?

That setting is only per-index, but ideally should change depending on the length of the query. Unfortunately you can't change it by passing an option to SearchApiQuery, e.g.
$query=new SearchApiQuery($index, array('http_method' => Apache_Solr_Service::METHOD_GET)); // Doesn't work.

While would be a fairly simple patch to pass the custom http_method to SearchApiSolrConnection, it's not really an ideal solution as:
- it only works for custom code
- it's an abstraction leak
- you don't know the length of the query that you're going to generate when you call execute(). (you'd need to approximate based of the number of parameters you were passing).

Secondary sources online suggest Tomcat and Jetty have a GET request limit of a little over 4,000 bytes, while Solr can handle 65kB.

I think the only sensible option is to automatically switch based on the query length. This could maybe be an additional 'AUTO' option when choosing the request method. I think we can just hard code the threshold to 4,000, since anyone who has configured Tomcat/Jetty to support larger requests probably wants all requests to be sent as GET, and if not then they can patch the module.

Would you accept a patch that did this? i.e.
- Introduce "auto" option.
- Override search method, and switch at 4,000 character query length.

Darn, had I known/thought of these issues earlier we probably shouldn't have introduced this option at all, but just implemented it the way you suggest. Now we'd have to change the connection class interface, and make the option mostly redundant … Still, I guess you're right, this is the only option that allows the advantages of both GET and POST to be utilized.

So, yes, I'd accept such a patch:

• Add an "Auto" option for the HTTP method which becomes the default. Change description accordingly.
• Change the code handling the method accordingly, as well as the search() method in both the connection class interface and implementation.
• Add a hidden variable for the query string length at which the connection class should switch to POST. Document that variable in README.txt.

Thanks for reporting this, and offering to write the patch!

Here is the promised patch.

When writing this I noticed that SearchApiSolrConnection::search() has a $method parameter, but it is ignored in favour of $this->method. Both were the same in my testing, and picking one or the other is definitely outside the scope of this issue, but I thought it was worth mentioning.

Thanks a lot, it already looks very good!

The mismatch between the search() method documentation and what it does (i.e., ignore $method) is really a pity, I didn't quite notice that. The best solution might be to let the method parameter default to NULL and only use it to override the property if it is set – but that would change the interface, which we should probably avoid.
Maybe removing the option from the connection class would be the best solution? It would at least not throw fatal errors for people overriding search(), but it could still cause problems for them. So maybe just documenting the erroneous/peculiar behavior is the best way to go here. It doesn't really matter in the standard use case in any case.
(Hm, or maybe invert my first suggestion? Let the option/property default to NULL, and use the parameter if that isn't set? That might make the most sense, since the parameter at least wouldn't be completely useless, and the standard use case (with the service class, which always passes the same constructor option and method parameter) would work the same. Also, I notice this error means that the HTTP method currently cannot be overridden like the hook_search_api_solr_query_alter() documentation claims – something we should probably fix. So maybe letting the option default to NULL and not setting it in the service class?)

Anyways, back to your patch. There just were a few very minor problems, mentioned below and fixed in the attached patch. If these changes are fine with you, we could just decide what we want to do about the property/parameter mismatch and then commit this.
Thanks again!

1. +++ b/includes/service.inc
   @@ -219,9 +219,10 @@ class SearchApiSolrService extends SearchApiAbstractService {
          '#options' => array(
   +        'AUTO' => 'AUTO',
            'POST' => 'POST',
            'GET' => 'GET',
          ),
   

   Not sure about this, but I think we should make "AUTO" translatable. "POST" and "GET" are defined HTTP constants, but "AUTO" could be translated.

2. +++ b/includes/solr_connection.inc
   @@ -226,8 +226,8 @@ class SearchApiSolrConnection implements SearchApiSolrConnectionInterface {
   -    // Make sure we always have a valid method set, default to POST.
   -    $this->method = $options['http_method'] == 'GET' ? 'GET' : 'POST';
   +    // Make sure we always have a valid method set, default to AUTO.
   +    $this->method = in_array($options['http_method'], array('GET', 'POST', 'AUTO')) ? $options['http_method'] : 'POST';
   

   Either do default to "AUTO", or don't change the comment. Since we should probably leave as much of the interface intact, even though defaulting to "AUTO" would make more sense, I think we should just keep defaulting to "POST". (This is also what it says in the method documentation.)

I don't have a strong opinion on where $method should come from. You should probably remove one or the other, but which one probably depends on what impact it will have on people using the API, what other plugins do, and whether people want to set this per-connection or per-query. I suggest we do this in a follow up so we make the right decision not the easy one.

Strictly speaking queries that will change the data (e.g. when indexing) should use POST. Should we add some logic for that to auto mode too?

I agree with both your changes. The latter was because I initially changed all defaults to AUTO, then thought it was probably better to leave the API-level ones alone.

I don't have a strong opinion on where $method should come from. You should probably remove one or the other, but which one probably depends on what impact it will have on people using the API, what other plugins do, and whether people want to set this per-connection or per-query. I suggest we do this in a follow up so we make the right decision not the easy one.

You're right: created #2113943: Resolve clash in HTTP method decision process.

Strictly speaking queries that will change the data (e.g. when indexing) should use POST. Should we add some logic for that to auto mode too?

update() (and, consequently, addDocuments()) already always uses POST, so there's nothing to do there I think.

I agree with both your changes. The latter was because I initially changed all defaults to AUTO, then thought it was probably better to leave the API-level ones alone.

Yes, I figured that – good thinking, thanks!

Anyways, if the above patch is fine with you, let's just wait shortly and then commit it.

Yep, I'm happy with that patch.

OK, committed.
Thanks again!