Export to GitHub

solr-php-client - issue #3

Plugable Query Writer Interface


Posted on Mar 9, 2009 by Quick Giraffe

Since Solr (1.4) now offers php output, I decided to get the class to use it. In addition, I tried to make it a little flexible so that you can switch which wt to use.

See the attached path. It should not affect current behavior in anyway. The constant in the class is now SOLR_DEFAULT_WRITER and is set to php (serialized php didn't work for me, PHP was also a royal PITA due to the array to object conversion stuff).

If you want to change back to using json, simply call setQueryWriter('json').

Attachments

Comment #1

Posted on Mar 9, 2009 by Grumpy Bear

Well, discussing with Jacob now - I think using the PHP type is a bit hazardous in terms of security.

There is a flag to pass to json_decode to get it to emit all arrays - otherwise we should try to fix upstream in Solr so that it emits serialized objects.

Comment #2

Posted on Mar 9, 2009 by Quick Giraffe

Added another patch. This sets the default back to json and also includes a fall back library for < 5.2 PHP installs.

Attachments

Comment #3

Posted on Mar 10, 2009 by Grumpy Bear

There seem to be a number of whitespace issues with the patch (tab vs space).

An alternative to that class would be the (likely better supported) Zend class:
http://framework.zend.com/manual/en/zend.json.html

However, I'm not sure in either case that one should include the external library with into this one - a PHP lib should be a last resort for those on PHP 5.1 who cannot install the PECL extension.

Rather than coercing the PHP response into objects, the extra param could be based to json_decode() to make it return arrays.

Comment #4

Posted on Mar 10, 2009 by Quick Giraffe

The Zend class is fine. It doesn't really matter to me. We just need some type of backup which works and quickly. This one seems to. If you feel so called to figure out the Zend plugin / licensing, go ahead but it seems like not an optimal use of time.

I feel we should bundle the backup along with the JSON writer. That's pretty much where it belongs. It's not a huge crime.

I don't like the json_decode issue because it breaks everyone's client code who is using this module. I feel like BC is important and this is not worth introducing risk.

Comment #5

Posted on Mar 10, 2009 by Grumpy Bear

here's a simple patch to require the Zend class if using PHP 5.1 without the PECL extension. Not actually tested yet, since I have PHP 5.2, nor am I even sure this should be committed - PHP 5.1 users could just patch or do this themselves.

Attachments

Comment #6

Posted on Mar 13, 2009 by Happy Bird

I believe Zend_Json_Decoder will use json_decode if it exists, so the Zend_Json patch would create infinite recursion.

As for the plugable parsers, that was actually how the client was originally written. I simplified it to just JSON after feedback from the Solr developers (Yonik and Grant). I'd prefer to not have to support more than one format. As for whether that format will be PHP, Serialized PHP, or JSON should be based on the native parsing speed of each and greatest overall support (across PHP versions and Solr versions).

Keep in mind also that the inclusion of json_decode might not be the only function in the code that requires PHP >= 5.2. I will have to run through it and check, but if there are other functions that require a PHP 5 version then the point of using PHP result formats for 4.3 compatibility may be moot. They should only be considered if they can marshalled by the server and parsed by the client faster than JSON responses.

Comment #7

Posted on Mar 18, 2009 by Quick Giraffe

Hi Donovan,

The only way we can harness our user base (which does include some 5.1 people) to test it is to apply a json compat layer.

Whatever we provide, it would be nice to bundle it with the library for usability purposes. Since the Zend Framework is BSD, this should be fine, right?

Best, Jacob

Status: New

Labels:
Type-Defect Priority-Medium